Multipath descriptors requires performing a deep copy, so a Clone
function that does that is added to miniscript::Node instead of the
current shallow copy.
Co-Authored-By: Antoine Poinsot <darosior@protonmail.com>
c0045e6cee06bc0029fb79b5a531aa1f2b817424 Add test for multipath miniscript expression (David Gumberg)
b4ac48090f259dbef567b49fa36a8bf192209710 descriptor: Use InferXOnlyPubkey for miniscript XOnly pubkey from script (Ava Chow)
4c50c21f6bfc1d88846be571055b481ab14b086f tests: Check ExpandPrivate matches for both parsed descriptors (Ava Chow)
092569e8580b7c2c13b6cc9d29bcb4c5e85bbb44 descriptor: Try the other parity in ConstPubkeyProvider::GetPrivKey() (Ava Chow)
Pull request description:
When a `ConstPubkeyProvider` is xonly, the stored pubkey does not necessarily have the correct parity bit. `ToPrivateString()` is correctly handling this by looking up the keys for both parity bits, but `GetPrivKey` does not. This results in not finding the private key when it is actually available if its pubkey has the other parity bit value.
To fix this, this key finding is refactored into `GetPrivKey()` so that its behavior is corrected, and `ToPrivateString()` is changed to use `GetPrivKey()` as well.
Additionally, the descriptor test checks are updated to include a check for `ExpandPrivate()` to verify that both the parsed public and private descriptors produce `SigningProvider`s with the same contents.
Fixes#31589
ACKs for top commit:
Pttn:
ACK c0045e6cee06bc0029fb79b5a531aa1f2b817424
davidgumberg:
utACK c0045e6cee
kevkevinpal:
Concept and Code review ACK [c0045e6](c0045e6cee)
furszy:
ACK c0045e6cee06bc0029fb79b5a531aa1f2b817424
theStack:
re-ACK c0045e6cee06bc0029fb79b5a531aa1f2b817424
rkrux:
Concept ACK c0045e6cee06bc0029fb79b5a531aa1f2b817424
Tree-SHA512: 3dcf2a802b996e0680a3f819075e5a689eb22e484c81ea79b40ec04197ee4ba3f6b9c87c45dfe8a847c9b805b2fd0fad77ffb92a93e65dc3aad74d69d9e3d97f
f6a6d912059c66792f48689632d2a7f14f8bdad9 test: add check for getting SigningProvider for a CPubKey (Sebastian Falbesoner)
62a95f5af9b998e241eb72c16ba581e77c480126 test: refactor: move `CreateDescriptor` helper to wallet test util module (Sebastian Falbesoner)
493656763f73e5ef1cfb979a513c12983dca99dd desc spkm: Return SigningProvider only if we have the privkey (Ava Chow)
Pull request description:
If we know about a pubkey that's in our descriptor, but we don't have the private key, don't return a SigningProvider for that pubkey.
This is specifically an issue for Taproot outputs that use the H point as the resulting PSBTs may end up containing irrelevant information because the H point was detected as a pubkey each unrelated descriptor knew about.
Split from #29675
ACKs for top commit:
fjahr:
ACK f6a6d912059c66792f48689632d2a7f14f8bdad9
theStack:
re-ACK f6a6d912059c66792f48689632d2a7f14f8bdad9
furszy:
utACK f6a6d912059. Only reviewed the actual change in detail, not the test commit.
Tree-SHA512: 30a196e611a0c5d9ebe5baf6d896caaa6af66f1615463dbb0c31e52604d53cf342922bb9967b3c697b47083d76b0485c77a5f545bd6381247c8bc44321c70f97
GetPrivKey() needs the same handling of all keyids for xonly keys that
ToPrivateString() does. Refactor that into GetPrivKey() and reuse it in
ToPrivateString() to resolve this.
366ae00b779acd59a61719422f0597acb17fb3e0 descriptor: Assume `ParseScript` is not being called with a P2WPKH context (brunoerg)
e36640859089baabc46f68217843f96a3ebdc20c descriptor: remove unreachable verification for `pkh` (brunoerg)
Pull request description:
This PR removes an unreachable verification in the `ParseScript` function. It returns an error if `pkh` is not being used at top level, sh, wsh or tr. However, any usage of `pkh` without these contexts will not reach this verification but other ones like "invalid keys" (e.g. `wpkh(pkh(L4gM1FBdyHNpkzsFh9ipnofLhpZRp2mwobpeULy1a6dBTvw8Ywtd))`).
ACKs for top commit:
davidgumberg:
crACK 366ae00b77
achow101:
ACK 366ae00b779acd59a61719422f0597acb17fb3e0
tdb3:
cr ACK 366ae00b779acd59a61719422f0597acb17fb3e0
sipa:
crACK 366ae00b779acd59a61719422f0597acb17fb3e0
Tree-SHA512: b954221a77eed623aeed5eb54f14e82c49540a151d3388831924caa7a784e48a2a975e418af1e13d491e4f8cded3b1797aa39e0e4e39e302a991105df09cdec0
For Span, iterators are just raw data pointers. However, for std::span
they are not.
This change makes it explicit where data pointers are expected.
Otherwise, there could be a compile error later on:
No known conversion from 'iterator' (aka '__normal_iterator<const std::byte *, std::span<const std::byte, 18446744073709551615>>') to 'std::byte *'.
This was missed during the original PR switching the nHashType argument
to int32_t in SignatureHash in bc52cda1f3c007bdf1ed00aa3011e207c7531017.
The problem was discovered after running into a linker error when
attempting to link this code as a static library using the header as a
declaration with a riscv32 bare metal toolchain. The compiler would
error with:
/opt/riscv-ilp32/lib/gcc/riscv32-unknown-elf/13.2.0/../../../../riscv32-unknown-elf/bin/ld: build_kernel_riscv/src/libbitcoin_consensus.a(interpreter.cpp.o): in function `GenericTransactionSignatureChecker<CTransaction>::CheckECDSASignature(std::vector<unsigned char, std::allocator<unsigned char> > const&, std::vector<unsigned char, std::allocator<unsigned char> > const&, CScript const&, SigVersion) const':
/home/user/bitcoin/build_kernel_riscv/./script/interpreter.cpp:2043:(.text._ZNK34GenericTransactionSignatureCheckerI12CTransactionE19CheckECDSASignatureERKSt6vectorIhSaIhEES6_RK7CScript10SigVersion[_ZNK34GenericTransactionSignatureCheckerI12CTransactionE19CheckECDSASignatureERKSt6vectorIhSaIhEES6_RK7CScript10SigVersion]+0xee): undefined reference to `uint256 SignatureHash<CTransaction>(CScript const&, CTransaction const&, unsigned int, int, long long const&, SigVersion, PrecomputedTransactionData const*)'
If we know about a pubkey that's in our descriptor, but we don't have
the private key, don't return a SigningProvider for that pubkey.
This is specifically an issue for Taproot outputs that use the H point
as the resulting PSBTs may end up containing irrelevant information
because the H point was detected as a pubkey each unrelated descriptor
knew about.
5e190cd11f6ed8b7ab4db0f01192de63deaf6fd7 Replace CScript _hex_v_u8 appends with _hex (Lőrinc)
cac846c2fbf6fc69bfc288fd387aa3f68d84d584 Allow CScript's operator<< to accept spans, not just vectors (Lőrinc)
c78d8ff4cb83506413bb73833fc5c04885d0ece8 prevector: avoid GCC bogus warnings in insert method (Lőrinc)
Pull request description:
Split out of https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1722326803.
Replace `_hex_v_u8` for `CScript` appends to `_hex`, to skip vector conversion before serializing to the `prevector` in `CScript`.
To enable both `unsigned char` and `std::byte` values, I've extracted the existing serialization to append the size & data in separate private methods to clarify that it does more than just a simple data insertion.
There were also discussion on eliminating the operators here completely to obviate when we're serializing fixed-size collections as raw bytes, and when we're prefixing them with their size - should also be done in a separate PR.
ACKs for top commit:
achow101:
ACK 5e190cd11f6ed8b7ab4db0f01192de63deaf6fd7
ryanofsky:
Code review ACK 5e190cd11f6ed8b7ab4db0f01192de63deaf6fd7. Looks good!
hodlinator:
re-ACK 5e190cd11f6ed8b7ab4db0f01192de63deaf6fd7
Tree-SHA512: 27a646629e017b2a05416d5eb964dda8b25b900d466221eff7bfa1339ded443e1c5c4cf8ff20cb3bba915a2603787a9fa6f6ec12bc0b9415d9eb07b57289192b
bc52cda1f3c007bdf1ed00aa3011e207c7531017 fix use int32_t instead of int type for risczero compile with (-march=rv32i, -mabi=ilp32) (Simon)
Pull request description:
When compile bitcoin by the toolchain(`riscv32-unknown-elf-g++`) from risc0 , the compiler argument is `-march=rv32i, -mabi=ilp32`, which will get the error which due to not serialize the value of type int .
```
blockbody-guest: cargo:warning=In file included from depend/bitcoin/src/hash.h:14,
blockbody-guest: cargo:warning= from depend/bitcoin/src/script/interpreter.h:9,
blockbody-guest: cargo:warning= from depend/bitcoin/src/script/interpreter.cpp:6:
blockbody-guest: cargo:warning=depend/bitcoin/src/serialize.h: In instantiation of 'void Serialize(Stream&, const T&) [with Stream = HashWriter; T = int]':
blockbody-guest: cargo:warning=depend/bitcoin/src/hash.h:144:20: required from 'HashWriter& HashWriter::operator<<(const T&) [with T = int]'
blockbody-guest: cargo:warning=depend/bitcoin/src/script/interpreter.cpp:1613:12: required from 'uint256 SignatureHash(const CScript&, const T&, unsigned int, int, const CAmount&, SigVersion, const PrecomputedTransactionData*) [with T = CTransaction; CAmount = long long int]'
blockbody-guest: cargo:warning=depend/bitcoin/src/script/interpreter.cpp:1664:36: required from 'bool GenericTransactionSignatureChecker<T>::CheckECDSASignature(const std::vector<unsigned char>&, const std::vector<unsigned char>&, const CScript&, SigVersion) const [with T = CTransaction]'
blockbody-guest: cargo:warning=depend/bitcoin/src/script/interpreter.cpp:1785:16: required from here
blockbody-guest: cargo:warning=depend/bitcoin/src/serialize.h:776:7: error: request for member 'Serialize' in 'a', which is of non-class type 'const int'
blockbody-guest: cargo:warning= 776 | a.Serialize(os);
```
--------------
### Reason
"The toolchain from RISC Zero defines int and int32_t as different types, although they have the same width. This means that `src/compat/assumptions.h` compiles fine; however, the templated serialization code cannot accept values of type int. Fix the compilation on RISC Zero by serializing int32_t instead of int values.
This patch will explicitly use the `int32_t` type instead of `int` to avoid errors when compiling with the risc0 toolchain. Additionally, this patch will not change any behavior on platforms where compilation was previously successful.
Fixes#30747
ACKs for top commit:
maflcko:
review-only ACK bc52cda1f3c007bdf1ed00aa3011e207c7531017
achow101:
ACK bc52cda1f3c007bdf1ed00aa3011e207c7531017
TheCharlatan:
ACK bc52cda1f3c007bdf1ed00aa3011e207c7531017
Tree-SHA512: ef880e7dfa1335bf2704ab17c0f506f17390b8259755674dfcd57131736492b2f4cfc36babda6902202b7c55a7513991e21f6634b0cd9b2b03baf4f1c0f8d78b
Extracted existing serialization to append size & data in separate private methods to clarify that it does more than just a simple data insertion.
* the C style casts were changed to static_cast
* `unsigned char` and `uint8_t` were changed to value_type for forward compatibility
* `data + sizeof(data)` was changed to `std::cend`
* data insertion (in AppendData) relies on pointer arithmetic now to enable both `std::span<const value_type>` and `std::span<const std::byte>` operators
* use uint32_t for data size instead of size_t
* used span instead of raw pointers in the new methods
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
Co-authored-by: Hodlinator <172445034+hodlinator@users.noreply.github.com>
These helpers haven't been used in production code since segwit was
merged more than eight years ago (see commit 605e8473, PR #8149),
so it seems appropriate to move them to the test utils module.
Can be reviewed via `--color-moved=dimmed-zebra`.
a0abcbd3822bd17a1d73c42ccd5b040a150b0501 doc: Mention multipath specifier (Ava Chow)
0019f61fc546b4d5f42eb4086f42560863fe0efb tests: Test importing of multipath descriptors (Ava Chow)
f97d5c137d605ac48f1122a836c9aa5f834957ba wallet, rpc: Allow importdescriptors to import multipath descriptors (Ava Chow)
32dcbca3fb918bc899a0637f876db31c3419aafd rpc: Allow importmulti to import multipath descriptors correctly (Ava Chow)
64dfe3ce4bed9ac168d0b08def8af7485db94ef1 wallet: Move internal to be per key when importing (Ava Chow)
16922455253f47fae0466c4ec6c3adfadcfe9182 tests: Multipath descriptors for scantxoutset and deriveaddresses (Ava Chow)
cddc0ba9a9dca3ca5873d768b3b504cdb2ab947b rpc: Have deriveaddresses derive receiving and change (Ava Chow)
360456cd221501fde3efe11bdba5c6d999dbb323 tests: Multipath descriptors for getdescriptorinfo (Ava Chow)
a90eee444c965bbd7bcddf9656eca9cee14c3aec tests: Add unit tests for multipath descriptors (Ava Chow)
1bbf46e2dae4599d04c79aaacf7c5db00b2e707f descriptors: Change Parse to return vector of descriptors (Ava Chow)
0d640c6f02bc20e5c1be773443dd74d8806d953b descriptors: Have ParseKeypath handle multipath specifiers (Ava Chow)
a5f39b103461a98689fd5d382e8da29037f55bea descriptors: Change ParseScript to return vector of descriptors (Ava Chow)
0d55deae157f4f8226b2419d55e7dc0dfb6e4aec descriptors: Add DescriptorImpl::Clone (Ava Chow)
7e86541f723d62c7ec6768f7f592c09ba2047d9e descriptors: Add PubkeyProvider::Clone (Ava Chow)
Pull request description:
It is convenient to have a descriptor which specifies both receiving and change addresses in a single string. However, as discussed in https://github.com/bitcoin/bitcoin/issues/17190#issuecomment-895515768, it is not feasible to use a generic multipath specification like BIP 88 due to combinatorial blow up and that it would result in unexpected descriptors.
To resolve that problem, this PR proposes a targeted solution which allows only a single pair of 2 derivation indexes to be inserted in the place of a single derivation index. So instead of two descriptor `wpkh(xpub.../0/0/*)` and `wpkh(xpub.../0/1/*)` to represent receive and change addresses, this could be written as `wpkh(xpub.../0/<0;1>/*)`. The multipath specifier is of the form `<NUM;NUM>`. Each `NUM` can have its own hardened specifier, e.g. `<0;1h>` is valid. The multipath specifier can also only appear in one path index in the derivation path.
This results in the parser returning two descriptors. The first descriptor uses the first `NUM` in all pairs present, and the second uses the second `NUM`. In our implementation, if a multipath descriptor is not provided, a pair is still returned, but the second element is just `nullptr`.
The wallet will not output the multipath descriptors (yet). Furthermore, when a multipath descriptor is imported, it is expanded to the two descriptors and each imported on its own, with the second descriptor being implicitly for internal (change) addresses. There is no change to how the wallet stores or outputs descriptors (yet).
Note that the path specifier is different from what was proposed. It uses angle brackets and the semicolon because these are unused characters available in the character set and I wanted to avoid conflicts with characters already in use in descriptors.
Closes#17190
ACKs for top commit:
darosior:
re-ACK a0abcbd3822bd17a1d73c42ccd5b040a150b0501
mjdietzx:
reACK a0abcbd3822bd17a1d73c42ccd5b040a150b0501
pythcoiner:
reACK a0abcbd
furszy:
Code review ACK a0abcbd
glozow:
light code review ACK a0abcbd3822
Tree-SHA512: 84ea40b3fd1b762194acd021cae018c2f09b98e595f5e87de5c832c265cfe8a6d0bc4dae25785392fa90db0f6301ddf9aea787980a29c74f81d04b711ac446c2
fad0cf6f2619df8df435a2da6da49eeb5510a10f refactor: Use std::ranges::equal in GetNetworkForMagic (MarcoFalke)
fadf0a7e15d66ba3230153e789b785e6cf8ab84c refactor: Remove Span operator==, Use std::ranges::equal (MarcoFalke)
Pull request description:
`std::span` removed the comparison operators, so it makes sense to remove them for the `Span` "backport" as well. Using `std::ranges::equal` also has the benefit that some `Span` temporary constructions can now be dropped.
This is required to move from `Span` toward `std::span`.
ACKs for top commit:
hodlinator:
Untested Code Review re-ACK fad0cf6
stickies-v:
ACK fad0cf6f2619df8df435a2da6da49eeb5510a10f
TheCharlatan:
ACK fad0cf6f2619df8df435a2da6da49eeb5510a10f
Tree-SHA512: 5b9d1826ceac2aabae2295bc89893dd23ac3a1cc0d41988331cdbdc21be531aa91795d5273819f349f79648c6c4f30ed31af6e7a3816153e92080061b92ffe00
fa7b9b99a2ed466d1765d748f4a59c2f581b974f refactor: Require std::input_iterator for all InputIterator in prevector (MarcoFalke)
d4444419001ca2af4c601a996461b67110645a52 refactor: Allow CScript construction from any std::input_iterator (MarcoFalke)
Pull request description:
Currently only (pre)vector iterators and raw pointers are accepted. However, this makes it harder to construct from input iterators provided by other classes, such as `std::span`.
Fix that.
ACKs for top commit:
delta1:
reACK fa7b9b9
achow101:
ACK fa7b9b99a2ed466d1765d748f4a59c2f581b974f
hodlinator:
ACK fa7b9b99a2ed466d1765d748f4a59c2f581b974f
ryanofsky:
Code review ACK fa7b9b99a2ed466d1765d748f4a59c2f581b974f
Tree-SHA512: 2760861f8bce42fb27dc0825e61621cb157f1ac3619a0834df38eb8319b6dcf9de43d90397a4c160f43340880c1553df638848e9057a27c792214331243ef4a5
Multipath specifiers are derivation path indexes of the form `<i;j;k;...>`
used for specifying multiple derivation paths for a descriptor.
Only one multipath specifier is allowed per PubkeyProvider.
This is syntactic sugar which is parsed into multiple distinct descriptors.
One descriptor will have all of the `i` paths, the second all of the `j` paths,
the third all of the `k` paths, and so on.
ParseKeypath will always return a vector of keypaths with the same size
as the multipath specifier. The callers of this function are updated to deal
with this case and return multiple PubkeyProviders. Their callers have
also been updated to handle vectors of PubkeyProviders.
Co-Authored-By: furszy <matiasfurszyfer@protonmail.com>
To prepare for returning multipath descriptors which will be a shorthand
for specifying multiple descriptors, change ParseScript's signature to return a
vector.
6714276d72244c2e2dbe9617f1341ba7fc06c0cc miniscript: Use `ToIntegral` instead of `ParseInt64` (brunoerg)
Pull request description:
Currently, miniscript code uses `ParseInt64` function for `after`, `older`, `multi` and `thresh` fragments. It means that a leading `+` or whitespace, among other things, are accepted into the fragments. However, these cases are not useful and cause Bitcoin Core to behave differently compared to other miniscript implementations (see https://github.com/brunoerg/bitcoinfuzz/issues/34). This PR fixes it.
ACKs for top commit:
achow101:
ACK 6714276d72244c2e2dbe9617f1341ba7fc06c0cc
tdb3:
cr ACK 6714276d72244c2e2dbe9617f1341ba7fc06c0cc
danielabrozzoni:
tACK 6714276d72244c2e2dbe9617f1341ba7fc06c0cc
darosior:
utACK 6714276d72244c2e2dbe9617f1341ba7fc06c0cc
Tree-SHA512: d9eeb93f380f346d636513eeaf26865285e7b0907b8ed258fe1e02153a9eb69d484c82180eb1c78b0ed77ad5f0e5b244be6672c2f890b1d9fddc9e844bee6dde
These outputs are called anchors, and allow
key-less anchor spends which are vsize-minimized
versus keyed anchors which require larger outputs
when creating and inputs when spending.
fe92c15f0c44d1405b9048306736bd0eae868506 script/sign: avoid duplicated signature verification after signing (Sebastian Falbesoner)
080089567ca766d4c1cde8ec0e5c7df48f566e07 bench: add benchmark for `SignTransaction` (Sebastian Falbesoner)
Pull request description:
This PR is a small performance improvement on the `SignTransaction` function, which is used mostly by the wallet (obviously) and the `signrawtransactionwithkey` RPC. The lower-level function `ProduceSignature` already calls `VerifyScript` internally as last step in order to check whether the signature data is complete:
daa56f7f66/src/script/sign.cpp (L568-L570)
If and only if that is the case, the `complete` field of the `SignatureData` is set to `true` accordingly and there is no need then to verify the script after again, as we already know that it would succeed.
This leads to a rough ~20% speed-up for `SignTransaction` for single-input ECDSA or Taproot transactions, according to the newly introduced `SignTransaction{ECDSA,Taproot}` benchmarks:
```
$ ./src/bench/bench_bitcoin --filter=SignTransaction.*
```
without commit 18185f4f578b8795fdaa75926630a691e9c8d0d4:
| ns/op | op/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 185,597.79 | 5,388.00 | 1.6% | 0.22 | `SignTransactionECDSA`
| 141,323.95 | 7,075.94 | 2.1% | 0.17 | `SignTransactionSchnorr`
with commit 18185f4f578b8795fdaa75926630a691e9c8d0d4:
| ns/op | op/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 149,757.86 | 6,677.45 | 1.4% | 0.18 | `SignTransactionECDSA`
| 108,284.40 | 9,234.94 | 2.0% | 0.13 | `SignTransactionSchnorr`
Note that there are already signing benchmarks in the secp256k1 library, but `SignTransaction` does much more than just the cryptographical parts, i.e.:
* calculate the unsigned tx's `PrecomputedTransactionData` if necessary
* apply Solver on the prevout scriptPubKey, fetch the relevant keys from the signing provider
* perform the actual signing operation (for ECDSA signatures, that could be more than once due to low-R grinding)
* verify if the signatures are correct by calling `VerifyScript` (more than once currently, which is fixed by this PR)
so it probably makes sense to also have benchmarks from that higher-level application perspective.
ACKs for top commit:
achow101:
ACK fe92c15f0c44d1405b9048306736bd0eae868506
furszy:
utACK fe92c15f0c44
glozow:
light review ACK fe92c15f0c44d1405b9048306736bd0eae868506
Tree-SHA512: b7225ff9e8a640ca5222dea5b2a463a0f9b9de704e4330b5b9a7bce2d63a1f4620575c474a8186f4708d7d9534eab55d000393d99db79c0cfc046b35f0a4a778
51fa26239af9bbfd44029aaf595cb4c6a8d4a75d refactor: Mark some static global vars as const (TheCharlatan)
39f9b80fba85d9818222c4d76e99ea1a804f5dda refactor: De-globalize last notified header index (TheCharlatan)
3443943f86678eb27d9895f3871fcf3945eb1c4f refactor: De-globalize validation benchmark timekeeping (TheCharlatan)
Pull request description:
In future, users of the kernel library might run multiple chainstates in parallel, or create and destroy multiple chainstates over the lifetime of a process. Having static, mutable variables could lead to state inconsistencies in these scenarios.
---
This pull request is part of the [libbitcoinkernel project](https://github.com/bitcoin/bitcoin/issues/27587).
ACKs for top commit:
dergoegge:
Code review ACK 51fa26239af9bbfd44029aaf595cb4c6a8d4a75d
maflcko:
ACK 51fa26239af9bbfd44029aaf595cb4c6a8d4a75d 🍚
tdb3:
code review ACK 51fa26239af9bbfd44029aaf595cb4c6a8d4a75d
Tree-SHA512: da91aa7ffa343325cabb8764ef03c8358845662cf0ba8a6cc1dd38e40e5462d88734f2b459c2de8e7a041551eda9143d92487842609f7f30636f61a0cd3c57ee
3333bae9b2a6c1ee2314d33361c93944c12001f9 tidy: modernize-use-equals-default (MarcoFalke)
Pull request description:
Prior to C++20, `modernize-use-equals-default` could have been problematic because it could turn a non-aggregate into an aggregate. The risk would be that aggregate initialization would be enabled where the author did not intend to enable it.
With C++20, aggregate for those is forbidden either way. (https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p1008r1.pdf)
So enabled it for code clarity, consistency, and possibly unlocking compiler optimizations. See https://clang.llvm.org/extra/clang-tidy/checks/modernize/use-equals-default.html
ACKs for top commit:
stickies-v:
ACK 3333bae9b2a6c1ee2314d33361c93944c12001f9
Tree-SHA512: ab42ff01be7ca7e7d8b4c6a485e68426f59627d83dd827cf292304829562348dc17a52ee009f5f6f3c1c2081d7166ffac4baef23197ebeba8de7767c6ddfe255
Move its ownership to the ChainstateManager class.
Next to simplifying usage of the kernel library by no longer requiring
manual setup of the cache prior to using validation code, it also slims
down the amount of memory allocated by BasicTestingSetup.
Use this opportunity to make SignatureCache RAII styled
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
Move its ownership to the ChainstateManager class.
Next to simplifying usage of the kernel library by no longer requiring
manual setup of the cache prior to using validation code, it also slims
down the amount of memory allocated by BasicTestingSetup.
c7376babd19d0c858fef93ebd58338abd530c1f4 doc: Clarify distinction between util and common libraries in libraries.md (Ryan Ofsky)
4f74c59334d496f28e1a5c0d84c412f9020b366f util: Move util/string.h functions to util namespace (Ryan Ofsky)
4d05d3f3b42a41525aa6ec44b90f543dfab53ecf util: add TransactionError includes and namespace declarations (Ryan Ofsky)
680eafdc74021c1e0893c3a62404e607fd4724f5 util: move fees.h and error.h to common/messages.h (Ryan Ofsky)
02e62c6c9af4beabaeea58fb1ea3ad0dc5094678 common: Add PSBTError enum (Ryan Ofsky)
0d44c44ae33434f366229c612d6edeedf7658963 util: move error.h TransactionError enum to node/types.h (Ryan Ofsky)
9bcce2608dd2515dc35a0f0866abc9d43903c795 util: move spanparsing.h to script/parsing.h (Ryan Ofsky)
6dd2ad47922694d2ab84bad4dac9dd442c5df617 util: move spanparsing.h Split functions to string.h (Ryan Ofsky)
23cc8ddff472d259605d7790ba98a1900e77efab util: move HexStr and HexDigit from util to crypto (TheCharlatan)
6861f954f8ff42c87ad638037adae86a5bd89600 util: move util/message to common/signmessage (Ryan Ofsky)
cc5f29fbea15d33e4d1aa95591253c6b86953fe7 build: move memory_cleanse from util to crypto (Ryan Ofsky)
5b9309420cc9721a0d5745b6ad3166a4bdbd1508 build: move chainparamsbase from util to common (Ryan Ofsky)
ffa27af24da81a97d6c4912ae0e10bc5b6f17f69 test: Add check-deps.sh script to check for unexpected library dependencies (Ryan Ofsky)
Pull request description:
Remove `fees.h`, `errors.h`, and `spanparsing.h` from the util library. Specifically:
- Move `Split` functions from `util/spanparsing.h` to `util/string.h`, using `util` namespace for clarity.
- Move remaining spanparsing functions to `script/parsing.h` since they are used for descriptor and miniscript parsing.
- Combine `util/fees.h` and `util/errors.h` into `common/messages.h` so there is a place for simple functions that generate user messages to live, and these functions are not part of the util library.
Motivation for this change is that the util library is a dependency of the kernel, and we should remove functionality from util that shouldn't be called by kernel code or kernel applications. These changes should also improve code organization and make functions easier to discover. Some of these same moves are (or were) part of #28690, but did not help with code organization, or made it worse, so it is better to move them and clean them up in the same PR so code only has to change one time.
ACKs for top commit:
achow101:
ACK c7376babd19d0c858fef93ebd58338abd530c1f4
TheCharlatan:
Re-ACK c7376babd19d0c858fef93ebd58338abd530c1f4
hebasto:
re-ACK c7376babd19d0c858fef93ebd58338abd530c1f4.
Tree-SHA512: 5bcef16c1255463b1b69270548711e7ff78ca0dd34e300b95e3ca1ce52ceb34f83d9ddb2839e83800ba36b200de30396e504bbb04fa02c6d0c24a16d06ae523d
In order to ensure that the change of nVersion to a uint32_t in the
previous commit has no effect, rename nVersion to version in this commit
so that reviewers can easily spot if a spot was missed or if there is a
check somewhere whose semantics have changed.
