6ee32aaaca4a46d42594bc16479868c76cc67fd8 test: signet tool genpsbt and solvepsbt commands (Sjors Provoost)
0a99d99fe4cb8432945d37c8e5d221715c538e4f signet: miner skips PSBT step for OP_TRUE (Sjors Provoost)
cdfb70e5a6a9235ea18dd2cb8148b757902dc08e signet: split decode_psbt miner helper (Sjors Provoost)
Pull request description:
[BIP325](https://github.com/bitcoin/bips/blob/master/bip-0325.mediawiki) mentions the following rule:
> In the special case where an empty solution is valid (ie scriptSig and scriptWitness are both empty) this additional commitment can optionally be left out. This special case is to allow non-signet-aware block generation code to be used to test a custom signet chain where the challenge is trivially true.
Such a signet can be created using e.g. `-signetchallenge=51` (`OP_TRUE`). However `contrib/signet/miner` won't omit the commitment.
This PR improves the miner by skipping the PSBT for known trivial scripts (just `OP_TRUE` and trivial pushes for now). This prevents it from appending the 4 byte signet header to the witness commitment, as allowed by the above rule.
---
Previously the script would fail with `PSBT signing failed`, making it difficult to mine. This is no longer the case.
ACKs for top commit:
achow101:
ACK 6ee32aaaca4a46d42594bc16479868c76cc67fd8
theStack:
re-ACK 6ee32aaaca4a46d42594bc16479868c76cc67fd8
danielabrozzoni:
ACK 6ee32aaaca4a46d42594bc16479868c76cc67fd8
Tree-SHA512: e47fbf471f2909286a6c1c073799ea388b9c19551afcce96cf9af45cc48d25c02f1e48e08861a88b604361e2c107a759d5baf393da8a37360de419f31651758a
86e1111239cdb39dd32cfb5178653c608fa30515 test: verify node skips loading legacy wallets during startup (furszy)
9f94de5bb54ff683bd4d3a7723617b34a4706bb6 wallet: init, don't error out when loading legacy wallets (furszy)
Pull request description:
Instead of failing during initialization and shutting down the app when encountering a legacy wallet, skip loading the wallet and notify the user accordingly.
This allows users to access migration functionalities without needing to manually remove the wallet from settings.json or resort to using the bitcoin-wallet utility.
This means that GUI users will be able to use the migration button, and bitcoin-cli users will be able to call the migratewallet RPC directly after init.
ACKs for top commit:
achow101:
ACK 86e1111239cdb39dd32cfb5178653c608fa30515
w0xlt:
ACK 86e1111239
Tree-SHA512: 85d594a503ee7a833a23754b71b6ba4869ca34ed802c9ac0cd7b2fa56978f5fcad84ee4bd3acdcc61cf8e7f08f0789336febc5d76beae1eebf7bd51462512b78
f3a444c45fb4bf4e51d53ebf1cf4c2631ded481c gui: Disallow loading legacy wallets (Ava Chow)
09955172f38a4bbfa6fc8ffba3b057c73e7e3bbe wallet, rpc: Give warning in listwalletdir for legacy wallets (Ava Chow)
Pull request description:
A new field `warnings` is added for each wallet in `listwalletdir`. If a legacy wallet is detected, the warning will contain a message that the wallet is a legacy wallet and will need to be migrated before it can be loaded.
In the GUI, the "Open Wallet" menu is changed to show legacy wallets greyed out with "(needs migration)" appended to their name to indicate to the user that the legacy wallet will need to be migrated.
ACKs for top commit:
maflcko:
lgtm ACK f3a444c45fb4bf4e51d53ebf1cf4c2631ded481c
adyshimony:
Test ACK [f3a444c](f3a444c45f)
furszy:
Code review ACK f3a444c45fb4bf4e51d53ebf1cf4c2631ded481c
w0xlt:
Code Review ACK f3a444c45f
Tree-SHA512: 496caec0ca37845487bd709e592240315eb23461fbd697e68a7fde8e4d9b74b48aab1212c88dbbcc8a107a896b824c2e1f69691068641812ae903f873fa2f22b
b1ea542ae651cec433910d8c727abc41f17a7678 test: test MAX_SCRIPT_SIZE for block validity (Greg Sanders)
Pull request description:
I don't believe there are direct tests for this.
ACKs for top commit:
achow101:
ACK b1ea542ae651cec433910d8c727abc41f17a7678
TheCharlatan:
ACK b1ea542ae651cec433910d8c727abc41f17a7678
theStack:
ACK b1ea542ae651cec433910d8c727abc41f17a7678
Tree-SHA512: 1d7d3eab9c54977844bf2ca1aa403b070aae0f818db2fb5cae367d1c4d12f1e403b6fdec224af769a2ebb648cbca8bfd0d7df5db2a89fccf256c9c244484eba2
84aa484d45e2fb3c1149941ef23779e4adb983d9 test: fix transaction_graph_test reorg test (Greg Sanders)
eaf44f3767846a25efaec70fb81279a623f0e419 test: check chainlimits respects on reorg (Greg Sanders)
47894367b583c06c53d568dc4a984d27bac8f742 functional test: add MAX_DISCONNECTED_TX_POOL_BYTES coverage (Greg Sanders)
Pull request description:
`DisconnectedBlockTransactions::LimitMemoryUsage()` has unit test coverage, but the default value end to end doesn't have coverage.
This test adds exercised coverage of memory limiting of the disconnect pool, and some basic behavior sanity checks.
Another test added is making sure chainlimits are being respected on reorg, and the expected transactions pruned.
Lastly, fix the existing test case which was using a deficient test via directly inducing reorgs with `invalidateblock`
ACKs for top commit:
maflcko:
re-ACK 84aa484d45e2fb3c1149941ef23779e4adb983d9 🚋
TheCharlatan:
ACK 84aa484d45e2fb3c1149941ef23779e4adb983d9
Tree-SHA512: f5cdb9647fadc8eb30352ce38de44064103825e5358787dfccd6416fa8faf6ceea42552fe2250b37d56271a6c3898b3912e1c028652da122f5c99304aafddb64
4df4df45d7bc2e8be99325d40cda936aab87c083 test: fix sync function in rpc_psbt.py (Martin Zumsande)
Pull request description:
Even though the block is created on `node2`, the sync is only between `node1` and `node0`. Accordingly the test fails if I put a sleep in `msg_type == NetMsgType::HEADERS` processing: In this case, `node1` and `node0` do not hear about the new block, the sync still passes because they are in sync with each other, and later on in the `test_input_confs_control` subtest, `node1` would generate a forked block instead of building on the previous one, leading to test failure.
Haven't seen this in the CI, but I ran into it on an experimental branch.
ACKs for top commit:
maflcko:
lgtm ACK 4df4df45d7bc2e8be99325d40cda936aab87c083
achow101:
ACK 4df4df45d7bc2e8be99325d40cda936aab87c083
Tree-SHA512: 1211ba0ad263ebcd0aa6ef7c856dec7ec6ca6010e1df705e7243f6c9d950ccca6df1275c36a73a83034f49ea8401e8f9800c05cdb74c39e860e7ebcaf2ce6ada
fab1e02086ceebd7d96417b7a386fe61158bfda9 refactor: Pass verification_progress into block tip notifications (MarcoFalke)
fa76b378e4b218fb4853088328b9b488de18dcd2 rpc: Round verificationprogress to exactly 1 for a recent tip (MarcoFalke)
faf6304bdfdf228354b4072b72f4c0ef90fdaade test: Use mockable time in GuessVerificationProgress (MarcoFalke)
Pull request description:
Some users really seem to care about this. While it shouldn't matter much, the diff is so trivial that it is probably worth doing.
Fixes#31127
One could also consider to split the field into two dedicated ones (https://github.com/bitcoin/bitcoin/issues/28847#issuecomment-1807115357), but this is left for a more involved follow-up and may also be controversial.
ACKs for top commit:
achow101:
ACK fab1e02086ceebd7d96417b7a386fe61158bfda9
pinheadmz:
ACK fab1e02086ceebd7d96417b7a386fe61158bfda9
sipa:
utACK fab1e02086ceebd7d96417b7a386fe61158bfda9
Tree-SHA512: a3c24e3c446d38fbad9399c1e7f1ffa7904490a3a7d12623b44e583b435cc8b5f1ba83b84d29c7ffaf22028bc909c7cec07202b825480449c6419d2a190938f5
The current test directly uses invalidatblock to trigger
mempool re-entry of transactions. Unfortunately, the
behavior doesn't match what a real reorg would look like. As
a result you get surprising behavior such as the mempool
descendant chain limits being exceeded, or if a fork is
greater than 10 blocks deep, evicted block transactions stop
being submitted back into in the mempool.
Fix this by preparing an empty fork chain, and then
continuing with the logic, finally submitting the fork chain
once the rest of the test is prepared. This triggers a more
typical codepath.
Also, extend the descendant limit to 100, like ancestor
limit.
fa4b8b16c37830bf7ee1419aae94e99f294f0e9f test: Add missing ipc subtree to lint (MarcoFalke)
Pull request description:
The subtree docs and lint checks list the subtrees in three places, making it hard to follow and update and easy to miss one.
Fix all issues by including the missing one and removing the list in one place.
ACKs for top commit:
l0rinc:
ACK fa4b8b16c37830bf7ee1419aae94e99f294f0e9f
davidgumberg:
ACK fa4b8b16c3
achow101:
ACK fa4b8b16c37830bf7ee1419aae94e99f294f0e9f
hebasto:
re-ACK fa4b8b16c37830bf7ee1419aae94e99f294f0e9f, only a [comment](https://github.com/bitcoin/bitcoin/pull/32623#discussion_r2109243775) has been addressed since my recent [review](https://github.com/bitcoin/bitcoin/pull/32623#pullrequestreview-2871162934).
BrandonOdiwuor:
Re-ACK fa4b8b16c37830bf7ee1419aae94e99f294f0e9f
Tree-SHA512: 97a39cf63be2707b2e03502546797ef639a611030e0a860618256c7c0683381be0f05c8bf850f9542a25266a52f58cef690abf9cc243498b6e026c7a6f9a52b2
a5ac43d98d1ad3ebed934f2c50208a85aae17e5e doc: Add release notes describing bitcoin wrapper executable (Ryan Ofsky)
258bda80c009a25d1f1bdeffccf9ed1ffde29cb2 doc: Mention bitcoin wrapper executable in documentation (Ryan Ofsky)
d2739d75c911c8bf73a4d3005c57add1ae4a67ae build: add bitcoin.exe to windows installer (Sjors Provoost)
ba649c00063a43b59a63db17b509179a658a8d9a ci: Run multiprocess tests through wrapper executable (Ryan Ofsky)
29bdd743bb843f8b8ed2e426b6df36e9d7e54215 test: Support BITCOIN_CMD environment variable (Ryan Ofsky)
9c8c68891b43053acfe7b8eb9d2e0d2bcfcb4e1e multiprocess: Add bitcoin wrapper executable (Ryan Ofsky)
5076d20fdb70a4bfafc4bdfe8293e347cb6bfa78 util: Add cross-platform ExecVp and GetExePath functions (Ryan Ofsky)
Pull request description:
Intended to make bitcoin command line features more discoverable and allow installing new multiprocess binaries in libexec/ instead of bin/ so they don't cause confusion.
Idea and implementation of this were discussed in https://github.com/bitcoin/bitcoin/issues/30983.
---
Initial implementation of this feature is deliberately minimal so the UX can evolve in response to feedback and there are not too many details to debate and discuss in a single PR. But many improvements are possible or planned:
- Adding manpage and bash completions.
- Showing nicer error messages that detect if an executable isn't installed and suggest how to fix [(comment)](https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2073194474)
- Showing wrapper command lines in subcommand in help output [(comment)](https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2077800405). This could be done conditionally as suggested in the comment or be unconditional.
- Showing wrapper command lines in subcommand error output. There is a bitcoin-cli error pointed out in [(comment)](https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2091152243) that is needlessly confusing.
- Integrating help so `bitcoin help subcommand` invokes `bitcoin subcommand -h`. `bitcoin -h subcommand` should also be supported and be equivalent [(comment)](https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2093116725)
- Adding support for `bitcoin-util` subcommands. Ideal interface would probably be more like `bitcoin grind` not `bitcoin util grind` but this has been punted for now. Supporting subcommands directly would require some ArgsManager modifications
- Adding a dedicated python functional test for the wrapper. Right now there is some CI coverage by setting the `BITCOIN_CMD` variable, but this doesn't cover things like the help output and version output, and support for different directory layouts.
- Better `--multiprocess` (`-m`) / `--monolithic` (`-M`) default selection. Right now, default is monolithic but it probably makes sense to chose more intelligently depending on whether -ipc options are enabled and what binaries are available.
- Maybe parsing `bitcoin.conf` and supporting options to control wrapper behavior like custom locations or preferences or aliases.
- Better command command line usability. Allow combining short options like (`-ah`). Allow fuzzy matching of subcommands or suggestions if you misspell. (suggested by stickies in review club)
- Not directly related to this PR but `bitcoin-cli named` implementation used by the wrapper should do a better job disambiguating named arguments from base64 arguments ending in = as pointed out in [(comment)](https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2091886628)
---
This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/issues/28722). A review club meeting for it took place in https://bitcoincore.reviews/31375
ACKs for top commit:
Sjors:
utACK a5ac43d98d1ad3ebed934f2c50208a85aae17e5e
achow101:
ACK a5ac43d98d1ad3ebed934f2c50208a85aae17e5e
vasild:
ACK a5ac43d98d1ad3ebed934f2c50208a85aae17e5e
theStack:
ACK a5ac43d98d1ad3ebed934f2c50208a85aae17e5e
ismaelsadeeq:
fwiw my last review implied an ACK a5ac43d98d1ad3ebed934f2c50208a85aae17e5e
hodlinator:
ACK a5ac43d98d1ad3ebed934f2c50208a85aae17e5e
Tree-SHA512: 570e6a4ff8bd79ef6554da3d01f36c0a7c6d2dd7dace8f8732eca98f4a8bc2284474a9beadeba783114fe2f3dd08b2041b3da7753bae0b7f881ec50668cb821f
f66b14d2ecb0db991d255be49d6ff6ec361569b5 test: fix pushdata scripts (Greg Sanders)
Pull request description:
The original scripts were done incorrectly,
so they are changed to represent two
different 2-byte pushes.
Fixes https://github.com/bitcoin/bitcoin/pull/32114#discussion_r2034051063
ACKs for top commit:
ajtowns:
ACK f66b14d2ecb0db991d255be49d6ff6ec361569b5
TheCharlatan:
Re-ACK f66b14d2ecb0db991d255be49d6ff6ec361569b5
Tree-SHA512: 0956124ee0d2e8b6a594f9feeb47c1f598c68e24d277e874f81a093268113e9da2c75a02863dbaab68b962063f7d910bfd10abe3ad33ec182bc21d72908f06e6
7bc64a8859c3644fdf2eeff59f72a778ae60ea3f test: properly check for per-tx sigops limit (Sebastian Falbesoner)
Pull request description:
Currently the per-tx sigops limit standardness check (bounded by `MAX_STANDARD_TX_SIGOPS_COST`, throwing "bad-txns-too-many-sigops" if exceeded):
3f83c744ac/src/validation.cpp (L925-L927)
is only indirectly tested with the much higher per-block consensus limit (`MAX_BLOCK_SIGOPS_COST`):
3f83c744ac/test/functional/data/invalid_txs.py (L236-L242)
I.e. an increase in the per-tx limit up to the per-block one would still pass all of our tests. Refine that by splitting up the invalid tx template `TooManySigops` in a per-block and a per-tx template.
The involved functional tests taking use of these templates are `feature_block.py` and `p2p_invalid_txs.py`. Can be tested by applying e.g.
```diff
diff --git a/src/policy/policy.h b/src/policy/policy.h
index 2151ec13dd..e5766d2a55 100644
--- a/src/policy/policy.h
+++ b/src/policy/policy.h
@@ -37,7 +37,7 @@ static constexpr unsigned int MIN_STANDARD_TX_NONWITNESS_SIZE{65};
/** Maximum number of signature check operations in an IsStandard() P2SH script */
static constexpr unsigned int MAX_P2SH_SIGOPS{15};
/** The maximum number of sigops we're willing to relay/mine in a single tx */
-static constexpr unsigned int MAX_STANDARD_TX_SIGOPS_COST{MAX_BLOCK_SIGOPS_COST/5};
+static constexpr unsigned int MAX_STANDARD_TX_SIGOPS_COST{MAX_BLOCK_SIGOPS_COST/5 + 4};
/** Default for -incrementalrelayfee, which sets the minimum feerate increase for mempool limiting or replacement **/
static constexpr unsigned int DEFAULT_INCREMENTAL_RELAY_FEE{1000};
/** Default for -bytespersigop */
diff --git a/test/functional/mempool_accept.py b/test/functional/mempool_accept.py
```
where the tests succeed on master, but fail on this PR.
(Found by diving deeper into the jungle of current sig-ops limit, as preparation for reviewing the [BIP 54](https://github.com/bitcoin/bips/blob/master/bip-0054.md) draft and related preparatory PRs like #32521).
ACKs for top commit:
fjahr:
tACK 7bc64a8859c3644fdf2eeff59f72a778ae60ea3f
tapcrafter:
tACK 7bc64a8859c3644fdf2eeff59f72a778ae60ea3f
darosior:
ACK 7bc64a8859c3644fdf2eeff59f72a778ae60ea3f
instagibbs:
crACK 7bc64a8859c3644fdf2eeff59f72a778ae60ea3f
Tree-SHA512: 1365409349664a76a1d46b2fa358c0d0609fb17fffdd549423d22b61749481282c928be3c2fb428725735c82d319b4279f703bde01e94e4aec14bab206abb8cf
800b7cc42ca63f2a6b245a4d327c7092289da6e1 cmake: Add missed `SSE41_CXXFLAGS` (Hennadii Stepanov)
028476e71fdcca5ea780fcfecfa7cf06c323beab cmake: Remove `ENABLE_ARM_SHANI` from `bitcoin-build-config.h` (Hennadii Stepanov)
1e900528d2450177a50eed15b36d2e6c226c2f4e cmake: Remove `ENABLE_X86_SHANI` from `bitcoin-build-config.h` (Hennadii Stepanov)
8689628e2e36d70aed16f4c44d6503888600ab90 cmake: Remove `ENABLE_AVX2` from `bitcoin-build-config.h` (Hennadii Stepanov)
a8e2342dca5e9cd771d883d4906cce6d2bb3de69 cmake: Remove `ENABLE_SSE41` from `bitcoin-build-config.h` (Hennadii Stepanov)
Pull request description:
`ENABLE_{SSE41,AVX2,X86_SHANI,ARM_SHANI}` are already conditionally defined for the [`bitcoin_crypto`](https://github.com/bitcoin/bitcoin/blob/master/src/crypto/CMakeLists.txt) target, and they are not used by any other targets. Defining them globally in `bitcoin-build-config.h` is therefore redundant.
Additionally, the previously missing `SSE41_CXXFLAGS` variable has been [added](https://github.com/bitcoin/bitcoin/pull/32550#issuecomment-2890918551).
ACKs for top commit:
fanquake:
ACK 800b7cc42ca63f2a6b245a4d327c7092289da6e1
Tree-SHA512: da792a0b780c67b432b09c9288ca98d62545315c721fed13510d1c11f8bb0cddd9a4ed7a009b4d052471dda19d0641bbc1eae4805fc306d23bf9b4ef510089c8
785e1407b0a39fef81a7b25554aab88d4cecd66b wallet: Use util::Error throughout AddWalletDescriptor (Ava Chow)
Pull request description:
#32023 changed `AddWalletDescriptor` to return `util::Error`, but did not change all of the failure cases to do so. This may result in some callers continuing when there was actually an error. Unify all of the failure cases to use `util::Error` so that all callers handle `AddWalletDescriptor` errors in the same way.
The encapsulated return type is changed from `ScriptPubKeyMan*` to `std::reference_wrapper<DescriptorScriptPubKeyMan>`. This avoids having a value that can be interpreted as a bool, and also removes the need to constantly dynamic_cast the returned value. The only kind of `ScriptPubKeyMan` that can come out of `AddWalletDescriptor` is a `DescriptorScriptPubKeyMan` anyways.
ACKs for top commit:
Sjors:
utACK 785e1407b0a39fef81a7b25554aab88d4cecd66b
ryanofsky:
Code review ACK 785e1407b0a39fef81a7b25554aab88d4cecd66b
furszy:
Code review ACK 785e1407b0a39fef81a7b25554aab88d4cecd66b
Tree-SHA512: 52a48263c8d4161a8c0419b7289c25b0986f8e3bcd10b639eeeb0b6862d08b6c5e70998d20070ab26b39ecd90ab83dc8b71c65d85f70626282cf8cc6abff50e7
ee045b61efc1479c1866b786661ef39a863677d0 rpc, psbt: Require sighashes match for descriptorprocesspsbt (Ava Chow)
2b7682c3729d4e054ac4260b344a75ad4b7239b3 psbt: use sighash type field to determine whether to remove non-witness utxos (Ava Chow)
28781b5f06709212934c521c513bb2e1a521a31f psbt: Add sighash types to PSBT when not DEFAULT or ALL (Ava Chow)
15ce1bd73f80e998f7402433572b695f589f7f42 psbt: Enforce sighash type of signatures matches psbt (Ava Chow)
1f71cd337ad75390a1f8810d6715f3634ed07e98 wallet: Remove sighash type enforcement from FillPSBT (Ava Chow)
4c7d767e49b2e709a2b00af92ca76e9f30e47aec psbt: Check sighash types in SignPSBTInput and take sighash as optional (Ava Chow)
a11825694856a2643e9600fa537182fbb597c107 script: Add IsPayToTaproot() (Ava Chow)
d6001dcd4ada5b64c8113450ed736a2581c97518 wallet: change FillPSBT to take sighash as optional (Ava Chow)
e58b680923b10f0690de9dcd34f17fbb8d6de5eb psbt: Return PSBTError from SignPSBTInput (Ava Chow)
2adfd815325713d64b9daa61c2f93061d27bd47d tests: Test PSBT sighash type mismatch (Ava Chow)
5a5d26d6123e0056656e406cd9f35aac6f71df4b psbt: Require ECDSA signatures to be validly encoded (Ava Chow)
Pull request description:
Currently, we do not add the sighash field to PSBTs at all, even when we have signed with a non-default sighash. This PR changes the behavior such that when we (attempt to) sign with a sighash other than DEFAULT or ALL, the sighash type field will be added to the PSBT to inform the later signers that a different sighash type was used by a signer. Notably, this is necessary for MuSig2 support as all signers must sign using the same sighash type, but the sighash is not provided in partial signatures.
Furthermore, because the sighash type can also be provided on the command line, we require that if both a command line sighash type and the sighash field is present, they must specify the same sighash type. However, this was being checked by the wallet, rather than the signing code, so the `descriptorprocesspsbt` RPC was not enforcing this restriction at all, and in fact ignored the sighash field entirely. This PR refactors the checking code so that the underlying PSBT signing function `SignPSBTInput` does the check.
ACKs for top commit:
theStack:
re-ACK ee045b61efc1479c1866b786661ef39a863677d0
rkrux:
re-ACK ee045b61efc1479c1866b786661ef39a863677d0
fjahr:
Code review ACK ee045b61efc1479c1866b786661ef39a863677d0
Tree-SHA512: 4ead5be1ef6756251b827f594beba868a145d75bf7f4ef6f15ad21f0ae4b8d71b38c83494e5a6b75f37fadd097178cddd93d614b962a2c72fc134f00ba2f74ae
faf55fc80b11f3d9b0b12c1b26a9612ea9ce9b40 doc: Remove ParseInt mentions in documentation (MarcoFalke)
33332829333b589420f8038541d04ec6970f051d refactor: Remove unused Parse(U)Int* (MarcoFalke)
fa84e6c36cb0accf87123ca4eb98f6219d55fb5e bitcoin-tx: Reject + sign in MutateTxDel* (MarcoFalke)
face2519fac9e840d52f0334d9079e664be7eb28 bitcoin-tx: Reject + sign in vout parsing (MarcoFalke)
fa8acaf0b993c879ee8c516baa36339ff7b72406 bitcoin-tx: Reject + sign in replaceable parsing (MarcoFalke)
faff25a558ab15b5d8eeea5dd4c9c0d76350051b bitcoin-tx: Reject + sign in locktime (MarcoFalke)
dddd9e5fe38b81f1af6b343661b65e16b0de7c60 bitcoin-tx: Reject + sign in nversion parsing (MarcoFalke)
fab06ac03788243847b799a3feaac56bc918fba9 rest: Use SAFE_CHARS_URI in SanitizeString error msg (MarcoFalke)
8888bb499dec79258b1857b404d72f93650503f4 rest: Reject + sign in /blockhashbyheight/ (MarcoFalke)
fafd43c69192fcb48a9e04d52eeb07fff15655d0 test: Reject + sign when parsing regtest deployment params (MarcoFalke)
fa123afa0ef752e8645bf695d121da66d8f1382b Reject + sign when checking -ipcfd (MarcoFalke)
fa479857ed234d54df31d33b60de14c6ffab3d6f Reject + sign in SplitHostPort (MarcoFalke)
fab4c2967d554ddbc15f732cea6cd190c547d89f net: Reject + sign when parsing subnet mask (MarcoFalke)
fa89652e68fc07fb6c9f3d9e34dc11d35f0cc1e1 init: Reject + sign in -*port parsing (MarcoFalke)
fa9c45577dfbae67535e4965b5660288557d3631 cli: Reject + sign in -netinfo level parsing (MarcoFalke)
fa980413257e2004a8d48a8be66c6d67f90b76ad refactor: Use ToIntegral in CreateFromDump (MarcoFalke)
fa23ed7fc24212d85cdc7f52b317906b37a1a120 refactor: Use ToIntegral in ParseHDKeypath (MarcoFalke)
Pull request description:
The legacy int parsing is problematic, because it accepts the `+` sign for unsigned integers. In all cases this is either:
* Useless, because the `+` sign was already rejected.
* Erroneous and inconsistent, when third party parsers reject it. (C.f. https://github.com/bitcoin/bitcoin/pull/32365)
* Confusing, because the `+` sign is neither documented, nor can it be assumed to be present.
Fix all issues by removing the legacy int parsing.
ACKs for top commit:
stickies-v:
re-ACK faf55fc80b
brunoerg:
code review ACK faf55fc80b11f3d9b0b12c1b26a9612ea9ce9b40
Tree-SHA512: a311ab6a58fe02a37741c1800feb3dcfad92377b4bfb61b433b2393f52ba89ef45d00940972b2767b213a3dd7b59e5e35d5b659c586eacdfe4e565a77b12b19f
4b2cd0b41ff4800c8801f2c44883eaec60a035fa test: check that creating a wallet does not log version info (Ava Chow)
39a483c8e9dcfe8ec243fa72269e1df9e75059ab test: Check that the correct versions are logged on wallet load (Ava Chow)
359ecd3704993422eb53e3da2a7d0bea2f575ab0 walletdb: Log the wallet version after it has been read from disk (Ava Chow)
Pull request description:
The wallet's version (in the minversion record) needs to be logged only after we have read it from disk. Otherwise, we always log the lowest version number of 10500 which is incorrect. Furthermore, it doesn't make sense to log the last client version number if the record didn't exist. This is a regression caused by #26021.
The wallet file version logging is moved inside of `LoadMinVersion` so that it is logged after the record is read. It will also log unconditionally if a version is read so that the version number is reported even when there is an error. The last client logging is split into its own log line that will only occur if a last client record is read. The only situation where we expect no version numbers to be logged is when a wallet is being created.
A test is added in the second commit to check that the version number is correctly logged on loading. This commit can be cherrypicked to master to verify that it fails there. The last commit adds an additional check that creating a new wallet does not log any version info at all.
ACKs for top commit:
laanwj:
Code review ACK 4b2cd0b41ff4800c8801f2c44883eaec60a035fa
janb84:
ACK 4b2cd0b41f
furszy:
ACK 4b2cd0b41ff4800c8801f2c44883eaec60a035fa
rkrux:
ACK 4b2cd0b41ff4800c8801f2c44883eaec60a035fa
Tree-SHA512: b30c76f414d87be6c14b42d2d3c8794a91a7e8601501f4c24641d51ff2b5c5144776563baf41ca1c38415844740b760b19a3e5791f78013b39984dfedd3b1de7
fa9198af55df74b0c19c9125d256ad4df83cf005 lint: Check for missing trailing newline (MarcoFalke)
fa2b2aa27c29fe810e296ef82126553b8f0d56e6 lint: Add archived notes to default excludes (MarcoFalke)
Pull request description:
A missing trailing newline is harmless, but a bit problematic:
* `git` shows a warning by default
* After another line is appended, the diff will be verbose and `git blame` will be wrong for the "untouched" line.
Fix the problems by just requiring what is already the default, see also 663a9cabf8/.editorconfig (L9) and 663a9cabf8/test/lint/test_runner/src/main.rs (L327)
ACKs for top commit:
l0rinc:
utACK fa9198af55df74b0c19c9125d256ad4df83cf005
fanquake:
ACK fa9198af55df74b0c19c9125d256ad4df83cf005
Tree-SHA512: d144eebdeee68fc3404aa4a66ecd5c130f907ed4b869bd300f6e9ed74d125561d1f4cdd6dd20d9e969471a7d007399f928f072d1c1f626275ca31f32bc23fdbc
32023 changed AddWalletDescriptor to return util::Error, but did not
change all of the failure cases to do so. This may result in some
callers continuing when there was actually an error. Unify all of the
failure cases to use util::Error so that all callers handle
AddWalletDescriptor errors in the same way.
The encapsulated return type is changed from ScriptPubKeyMan* to
std::reference_wrapper<DescriptorScriptPubKeyMan>. This avoids having a
value that can be interpreted as a bool, and also removes the need to
constantly dynamic_cast the returned value. The only kind of
ScriptPubKeyMan that can come out of AddWalletDescriptor is a
DescriptorScriptPubKeyMan anyways.
30a94b1ab9ae850d55cb9eb606a06890437bc75e test, wallet: Remove concurrent writes test (Ava Chow)
b44b7c03fef01e0b5db704e50762b3d16b3da69e wallet: Write best block record on unload (Ava Chow)
876a2585a8b69e12ac171a0d9ff5aab864067c42 wallet: Remove unnecessary database Close step on shutdown (Ava Chow)
98a1a5275c8c395fe47ff7f10109d75b06bc391d wallet: Remove chainStateFlushed (Ava Chow)
7fd3e1cf0c88553e0722048ce488f265883558e7 wallet, bench: Write a bestblock record in WalletMigration (Ava Chow)
6d3a8b195a826448c021dd189255ca41ba70cc5a wallet: Replace chainStateFlushed in loading with SetLastBlockProcessed (Ava Chow)
7bacabb204b6c34f9545f0b37e2c66296ad2c0de wallet: Update best block record after block dis/connect (Ava Chow)
Pull request description:
Implements the idea discussed in https://github.com/bitcoin/bitcoin/pull/29652#issuecomment-2010579484
Currently, `m_last_block_processed` and `m_last_block_processed_height` are not guaranteed to match the block locator stored in the wallet, nor do either of those fields actually represent the last block that the wallet is synced up to. This is confusing and unintuitive.
This PR changes those last block fields to be updated whenever the wallet makes a change to the db for new transaction state found in new blocks. Whenever a block is received that contains a transaction relevant to the wallet, the last block locator will now be written to disk. Furthermore, every block disconnection will now write an updated locator.
To ensure that the locator is relatively recent and loading rescans are fairly quick in the event of unplanned shutdown, it is also now written every 144 blocks (~1 day). Additionally it is now written when the wallet is unloaded so that it is accurate when the wallet is loaded again.
Lastly, the `chainstateFlushed` notification in the wallet is changed to be a no-op. The best block locator record is no longer written when `chainstateFlushed` is received from the node since it should already be mostly up to date.
ACKs for top commit:
rkrux:
ACK 30a94b1ab9ae850d55cb9eb606a06890437bc75e
mzumsande:
Code Review ACK 30a94b1ab9ae850d55cb9eb606a06890437bc75e
ryanofsky:
Code review ACK 30a94b1ab9ae850d55cb9eb606a06890437bc75e. Only changes since last review are using WriteBestBlock method more places and updating comments.
Tree-SHA512: 46117541f8aaf13dde57430e813b4bbbd5e146e2632769675803c8e65a82f149a7cc6026489a127d32684b90124bd2b7c28216dbcfa6a47447300e8f3814e029
Descriptor wallets are already created by default since v23.0, but
since the recent legacy wallet removal this parameter *must* be True
(see commit 9f04e02ffaee0fe64027dc56c7bea3885254321a), i.e. still
passing it wouldn't contain any information for test readers
anymore. So simply drop them in the functional tests in order to
reduce code bloat.
-BEGIN VERIFY SCRIPT-
sed -i 's/, descriptors=True//g' $(git ls-files -- 'test/functional' ':(exclude)test/functional/wallet_backwards_compatibility.py')
sed -i '/descriptors=True,/d' ./test/functional/mempool_persist.py
-END VERIFY SCRIPT-
b104d442277090337ce405d92f1398b7cc9bcdb7 test: Remove RPCOverloadWrapper (Ava Chow)
4d32c19516fdb65b364638fa088b8d7167b438b6 test: Replace importpubkey (Ava Chow)
fe838dd391be669ccd0765b95f81c25fecfd3636 test: Replace usage of addmultisigaddress (Ava Chow)
d3142077794f4e910d3cdc749020d725e30feb24 test: Replace usage of importaddress (Ava Chow)
fcc457573f9b39e6f173a4f51c45d7dbb47e7ab0 test: Replace importprivkey with wallet_importprivkey (Ava Chow)
94c87bbbd06eb9a57930b9f59315533cfbe8f460 test: Remove unnecessary importprivkey from wallet_createwallet (Ava Chow)
Pull request description:
`RPCOverloadWrapper` implemented overloads for legacy wallet only RPCs so that the same function call could be used within tests for both legacy wallets and descriptor wallets. With legacy wallets now removed, there is no need to continue to have these overloads.
For `importaddress`, `addmultisigaddress`, and `importpubkey`, the uses of these are converted to `importdescriptors`.
For `importprivkey`, a new helper function `wallet_imporprivkey` is introduced that does what the overload did. This is mainly to reduce verbosity as `importprivkey` was more widely used throughout the tests.
Some tests that used these RPCs are now also no longer relevant and have been removed.
ACKs for top commit:
Sjors:
ACK b104d442277090337ce405d92f1398b7cc9bcdb7
pablomartin4btc:
cr ACK b104d442277090337ce405d92f1398b7cc9bcdb7
rkrux:
ACK b104d442277090337ce405d92f1398b7cc9bcdb7
w0xlt:
ACK b104d44227
Tree-SHA512: ded2f73829e2ce28466d4a9738eb382783ad990daee5d1859dbc4d354e6f8eec0c483ed5ecb1287fe0dd24ac332065b733a30d71b126b841bd7cd49e9a094b6d
When dealing with URI parts, it seems more consistent to use
corresponding SAFE_CHARS_URI mode in error messages.
Co-Authored-By: stickies-v <stickies-v@protonmail.com>
