The "connect to ourself" detection logic has been first introduced
by Satoshi in October 2009, together with a couple of other changes
and a version bump to "v0.1.6 BETA" (see commit
cc0b4c3b62).
73f0a6cbd0 doc: detail -rpccookieperms option (willcl-ark)
d2afa2690c test: add rpccookieperms test (willcl-ark)
f467aede78 init: add option for rpccookie permissions (willcl-ark)
7df03f1a92 util: add perm string helper functions (willcl-ark)
Pull request description:
This PR picks up #26088 by aureleoules which adds a bitcoind launch option `-rpccookieperms` to set the file permissions of the cookie generated by bitcoin core.
Example usage to make the generated cookie group-readable: `./src/bitcoind -rpccookieperms=group`.
Accepted values for `-rpccookieperms` are `[owner|group|all]`. We let `fs::perms` handle platform-specific permissions changes.
ACKs for top commit:
achow101:
ACK 73f0a6cbd0
ryanofsky:
Code review ACK 73f0a6cbd0. Main change since last review is no longer throwing a skip exception in the rpc test on windows, so other checks can run after it, and overall test result is passing, not skipped. Also were clarifying renames and documentation improvements.
tdb3:
cr ACK 73f0a6cbd0
Tree-SHA512: e800d59a44aca10e1c58ca69bf3fdde9f6ccf5eab4b7b962645af6d6bc0cfa3a357701e409c8c60d8d7744fcd33a91e77ada11790aa88cd7811ef60fab86ab11
72b226882f wallet: notify when preset + automatic inputs exceed max weight (furszy)
Pull request description:
Small change. Found it while finishing my review on #29523. This does not interfere with it.
Basically, we are erroring out early when the automatic coin selection process exceeds the maximum weight, but we are not doing so when the user-preselected inputs combined with the wallet-selected inputs exceed the maximum weight.
This change avoids signing all inputs before erroring out and introduces test coverage for `fundrawtransaction`.
ACKs for top commit:
achow101:
ACK 72b226882f
tdb3:
re ACK for 72b226882f
rkrux:
tACK [72b2268](72b226882f)
ismaelsadeeq:
utACK 72b226882f
Tree-SHA512: d77be19231023383a9c79a5d66b642dcbc6ebfc31a363e0b9f063c44898720a7859ec211cdbc0914ac7a3bfdf15e52fb8fc20d97f171431f70492c0f159dbc36
a9716c53f0 rpc: call IsInitialBlockDownload via miner interface (Sjors Provoost)
dda0b0834f rpc: minize getTipHash() calls in gbt (Sjors Provoost)
7b4d3249ce rpc: call processNewBlock via miner interface (Sjors Provoost)
9e228351e7 rpc: getTransactionsUpdated via miner interface (Sjors Provoost)
64ebb0f971 Always pass options to BlockAssembler constructor (Sjors Provoost)
4bf2e361da rpc: call CreateNewBlock via miner interface (Sjors Provoost)
404b01c436 rpc: getblocktemplate getTipHash() via Miner interface (Sjors Provoost)
d8a3496b5a rpc: call TestBlockValidity via miner interface (Sjors Provoost)
8ecb681678 Introduce Mining interface (Sjors Provoost)
Pull request description:
Introduce a `Mining` interface for the `getblocktemplate`, `generateblock` and other mining RPCs to use now, and for Stratum v2 to use later.
Suggested here: https://github.com/bitcoin/bitcoin/pull/29346#issuecomment-2108528652
The selection of methods added to the interface is mostly based on what the Template Provider in #29432 uses. It could be expanded further so that `rpc/mining.cpp` no longer needs `EnsureMemPool` and `EnsureChainman`.
This PR should be a pure refactor.
ACKs for top commit:
tdb3:
re ACK a9716c53f0
itornaza:
Code review and std-tests ACK a9716c53f0
ryanofsky:
Code review ACK a9716c53f0 with one minor suggestion in case you update. Only changes since last review were other small changes to the interface.
Tree-SHA512: cf97f87d6e9ed89da3835a0730da3b24a7b14c8605ea221149103a5915e79598cf082a95f2bc88e33f1c450e3d4aad88aed1163a29195acca88bcace055af724
6eecba475e net_processing: make MaybePunishNodeFor{Block,Tx} return void (Pieter Wuille)
ae60d485da net_processing: remove Misbehavior score and increments (Pieter Wuille)
6457c31197 net_processing: make all Misbehaving increments = 100 (Pieter Wuille)
5120ab1478 net_processing: drop 8 headers threshold for incoming BIP130 (Pieter Wuille)
944c54290d net_processing: drop Misbehavior for unconnecting headers (Pieter Wuille)
9f66ac7cf1 net_processing: do not treat non-connecting headers as response (Pieter Wuille)
Pull request description:
So far, discouragement of peers triggers when their misbehavior score exceeds 100 points. Most types of misbehavior increment the score by 100, triggering immediate discouragement, but some types do not. This PR makes all increments equal to either 100 (meaning any misbehavior will immediately cause disconnection and discouragement) or 0 (making the behavior effectively unconditionally allowed), and then removes the logic for score accumulation.
This simplifies the code a bit, but also makes protocol expectations clearer: if a peer misbehaves, they get disconnected. There is no good reason why certain types of protocol violations should be permitted 4 times (howmuch=20) or 9 times (howmuch=10), while many others are never allowed. Furthermore, the distinction between these looks arbitrary.
The specific types of misbehavior that are changed to 100 are:
* Sending us a `block` which does not connect to our header tree (which necessarily must have been unsollicited). [used to be score 10]
* Sending us a `headers` with a non-continuous headers sequence. [used to be score 20]
* Sending us more than 1000 addresses in a single `addr` or `addrv2` message [used to be score 20]
* Sending us more than 50000 invs in a single `inv` message [used to be score 20]
* Sending us more than 2000 headers in a single `headers` message [used to be score 20]
The specific types of misbehavior that are changed to 0 are:
* Sending us 10 (*) separate BIP130 headers announcements that do not connect to our block tree [used to be score 20]
* Sending us more than 8 headers in a single `headers` message (which thus does not get treated as a BIP130 announcement) that does not connect to our block tree. [used to be score 10]
I believe that none of these behaviors are unavoidable, except for the one marked (*) which can in theory happen still due to interaction between BIP130 and variations in system clocks (the max 2 hour in the future rule). This one has been removed entirely. In order to remove the impact of the bug it was designed to deal with, without relying on misbehavior, a separate improvement is included that makes `getheaders`-tracking more accurate.
In another unrelated improvement, this also gets rid of the 8 header limit heuristic to determine whether an incoming non-connecting `headers` is a potential BIP130 announcement, as this rule is no longer needed to prevent spurious Misbehavior. Instead, any non-connecting `headers` is now treated as a potential announcement.
ACKs for top commit:
sr-gi:
ACK [6eecba4](6eecba475e)
achow101:
ACK 6eecba475e
mzumsande:
Code Review ACK 6eecba475e
glozow:
light code review / concept ACK 6eecba475e
Tree-SHA512: e11e8a652c4ec048d8961086110a3594feefbb821e13f45c14ef81016377be0db44b5311751ef635d6e026def1960aff33f644e78ece11cfb54f2b7daa96f946
94ed4fbf8e Add release note for size 2 package rbf (Greg Sanders)
afd52d8e63 doc: update package RBF comment (Greg Sanders)
6e3c4394cf mempool: Improve logging of replaced transactions (Greg Sanders)
d3466e4cc5 CheckPackageMempoolAcceptResult: Check package rbf invariants (Greg Sanders)
316d7b63c9 Fuzz: pass mempool to CheckPackageMempoolAcceptResult (Greg Sanders)
4d15bcf448 [test] package rbf (glozow)
dc21f61c72 [policy] package rbf (Suhas Daftuar)
5da3967815 PackageV3Checks: Relax assumptions (Greg Sanders)
Pull request description:
Allows any 2 transaction package with no in-mempool ancestors to do package RBF when directly conflicting with other mempool clusters of size two or less.
Proposed validation steps:
1) If the transaction package is of size 1, legacy rbf rules apply.
2) Otherwise the transaction package consists of a (parent, child) pair with no other in-mempool ancestors (or descendants, obviously), so it is also going to create a cluster of size 2. If larger, fail.
3) The package rbf may not evict more than 100 transactions from the mempool(bip125 rule 5)
4) The package is a single chunk
5) Every directly conflicted mempool transaction is connected to at most 1 other in-mempool transaction (ie the cluster size of the conflict is at most 2).
6) Diagram check: We ensure that the replacement is strictly superior, improving the mempool
7) The total fee of the package, minus the total fee of what is being evicted, is at least the minrelayfee * size of the package (equivalent to bip125 rule 3 and 4)
Post-cluster mempool this will likely be expanded to general package rbf, but this is what we can safely support today.
ACKs for top commit:
achow101:
ACK 94ed4fbf8e
glozow:
reACK 94ed4fbf8e via range-diff
ismaelsadeeq:
re-ACK 94ed4fbf8e
theStack:
Code-review ACK 94ed4fbf8e
murchandamus:
utACK 94ed4fbf8e
Tree-SHA512: 9bd383e695964f362f147482bbf73b1e77c4d792bda2e91d7f30d74b3540a09146a5528baf86854a113005581e8c75f04737302517b7d5124296bd7a151e3992
881724d443 test: Added test coverage to listsinceblock rpc (kevkevinpal)
Pull request description:
This change is meant to add test coverage to this rpc error https://github.com/bitcoin/bitcoin/blob/master/src/wallet/rpc/transactions.cpp#L666C53-L666C79
This is done by renaming the first block in the blocks folder
---
Doing a quick grep for the error code in our functional tests leads to zero results
`grep -nri "Can't read block from disk" ./test/functional/`
ACKs for top commit:
achow101:
ACK 881724d443
tdb3:
re ACK for 881724d443
rkrux:
tACK [881724](881724d443)
Tree-SHA512: c5dff20cf014d0181f49d6b161f1364e1c6b79e8661047f77f07e21e59f4d1f2fd6f745538c8fc5bd6d4244650a840dd64d184634366f7c21fa67141a60af44a
5cf0a1f230 test: add `createmultisig` P2MS encoding test for all n (1..20) (Sebastian Falbesoner)
0570d2c204 test: add unit test for `keys_to_multisig_script` (Sebastian Falbesoner)
0c41fc3fa5 test: fix `keys_to_multisig_script` (P2MS) helper for n/k > 16 (Sebastian Falbesoner)
Pull request description:
While reviewing #28307, I noticed that the test framework's `key_to_multisig_script` helper (introduced in #23305) is broken for pubkey count (n) and threshold (k) values larger than 16. This is due to the implementation currently enforcing a direct single-byte data push (using `CScriptOp.encode_op_n`), which obviously fails for values 17+. Fix that by passing the numbers directly to the CScript list, where it's automatically converted to minimally-encoded pushes (see class method `CScript.__coerce_instance`, branch `isinstance(other, int)`).
The second commit adds a unit test to ensure that the encoding is correct.
ACKs for top commit:
achow101:
ACK 5cf0a1f230
tdb3:
ACK 5cf0a1f230
rkrux:
reACK [5cf0a1f](5cf0a1f230)
Tree-SHA512: 4168a165c3f483ec8e37a27dba1628a7ea0063545a2b7e74d9e20d753fddd7e33d37e1a190434fa6dca39adf9eef5d0211f7a0c1c7b44979f0a3bb350e267562
Adds argument --resultsfile to test_runner.py.
Writes comma-separated functional test name, status,
and duration to the file provided with the argument.
Also fixes minor typo in test_runner.py
f58beabe75 test: bumpfee with user specified fee_rate ignores walletIncrementalRelayFee (ismaelsadeeq)
436e88f433 bumpfee: ignore WALLET_INCREMENTAL_RELAY_FEE when user specifies fee rate (ismaelsadeeq)
Pull request description:
Fixes#26973
When using the `bumpfee` RPC and manually specifying `fee_rate`, there should be no requirement that the new fee must be at least the sum of the original fee and `incrementalFee` (maximum of `relayIncrementalFee` and `WALLET_INCREMENTAL_RELAY_FEE`).
This restriction should only apply when user did not specify `fee_rate`.
> because the GUI doesn't let the user specify the new fee rate yet (https://github.com/bitcoin-core/gui/issues/647), it would be very annoying to have to bump 20 times to increment by 20 sat/vbyte.
The restriction should instead be the new fee must be at least the sum of the original fee and `incrementalFee` (`relayIncrementalFee`)
ACKs for top commit:
achow101:
ACK f58beabe75
murchandamus:
ACK f58beabe75
Tree-SHA512: 193259f87173b7d5a8e68e0e29f2ca7e75c550e3cf0dee3d6d822b5b1e07c2e6dec0bfc8fb435855736ebced97a10dbdbfef72e8c5abde06fdefcba122f2e7f1
e2779ce98b test: cover more errors for `signrawtransactionwithkey` RPC (brunoerg)
Pull request description:
This PR adds test coverage for the following errors for the `signrawtransactionwithkey` RPC:
- Invalid private key
- TX decode failed
For reference: https://maflcko.github.io/b-c-cov/total.coverage/src/rpc/rawtransaction.cpp.gcov.html
ACKs for top commit:
maflcko:
ACK e2779ce98b
kevkevinpal:
ACK [e2779ce](e2779ce98b)
tdb3:
ACK e2779ce98b
BrandonOdiwuor:
Code Review ACK e2779ce98b
Tree-SHA512: 41c7e990684b60645cf4ccec8aad5ebbe61da221871eb3c1685b2bb1eebda58b29358502cb1525b7c7a2b612e2bebf449ed0bae14ab663b4641c528a9c013b5b
0fcbfdb7ad Support running individual lint checks (David Gumberg)
Pull request description:
This PR was split out from #29965:
Adds support for running individual tests in the rust lint suite by passing `--lint=LINT_TO_RUN` to the lint runner. This PR also adds a corresponding help message.
When running with `cargo run`, arguments after a double dash (`--`) are passed to the binary instead of the cargo command. For example, in order to run the linter check that tabs are not used as whitespace:
```console
cd test/lint/test_runner && cargo run -- --lint=tabs_whitespace
```
ACKs for top commit:
maflcko:
ACK 0fcbfdb7ad
achow101:
ACK 0fcbfdb7ad
marcofleon:
Tested ACK 0fcbfdb7ad. Ran `cargo run` with various of the individual tests and with bad input. Also ran it with no arguments. Everything works as expected and help message looks good.
Tree-SHA512: 48fe4aa9fbb2acef5f8e3c17382ae22e0e350ae6ad9aeeb1a3c0a9192de98809f98728e32b8db24a36906ace999e35626ebd6cb2ca05f74146d21e9b6fb14615
429ec1aaaa refactor: Rename CTransaction::nVersion to version (Ava Chow)
27e70f1f5b consensus: Store transaction nVersion as uint32_t (Ava Chow)
Pull request description:
Given that the use of a transaction's nVersion is always as an unsigned int, it doesn't make sense to store it as signed and then cast it to unsigned everywhere it is used and displayed.
Since a few alternative implementations have recently been revealed to have made an error with this signedness that would have resulted in consensus failure, I think it makes sense for us to just make this always unsigned to make it clear that the version is treated as unsigned. This would also help us avoid future potential issues with signedness of this value.
I believe that this is safe and does not actually change what transactions would or would not be considered both standard and consensus valid. Within consensus, the only use of the version in consensus is in BIP68 validation which was already casting it to uint32_t. Within policy, although it is used as a signed int for the transaction version number check, I do not think that this change would change standardness. Standard transactions are limited to the range [1, 2]. Negative numbers would have fallen under the < 1 condition, but by making it unsigned, they are still non-standard under the > 2 condition.
Unsigned and signed ints are serialized and unserialized the same way so there is no change in serialization.
ACKs for top commit:
maflcko:
ACK 429ec1aaaa 🐿
glozow:
ACK 429ec1aaaa
shaavan:
ACK 429ec1aaaa💯
Tree-SHA512: 0bcd92a245d7d16c3665d2d4e815a4ef28207ad4a1fb46c6f0203cdafeab1b82c4e95e4bdce7805d80a4f4a46074f6542abad708e970550d38a00d759e3dcef1
47f705b33f tests: add fuzz tests for BitSet (Pieter Wuille)
59a6df6bd5 util: add BitSet (Pieter Wuille)
Pull request description:
Extracted from #30126.
This introduces the `BitSet` data structure, inspired by `std::bitset`, but with a few features that cannot be implemented on top without efficiency loss:
* Finding the first set bit (`First`)
* Finding the last set bit (`Last`)
* Iterating over all set bits (`begin` and `end`).
And a few other operators/member functions that help readability for #30126:
* `operator-` for set subtraction
* `Overlaps()` for testing whether intersection is non-empty
* `IsSupersetOf()` for testing (non-strict) supersetness
* `IsSubsetOf()` for testing (non-strict) subsetness
* `Fill()` to construct a set with all numbers from 0 to n-1, inclusive
* `Singleton()` to construct a set with one specific element.
Everything is tested through a simulation-based fuzz test that compares the behavior with normal `std::bitset` equivalent operations.
ACKs for top commit:
instagibbs:
ACK 47f705b33f
achow101:
ACK 47f705b33f
cbergqvist:
re-ACK 47f705b33f
theStack:
Code-review ACK 47f705b33f
Tree-SHA512: e451bf4b801f193239ee434b6b614f5a2ac7bb49c70af5aba24c2ac0c54acbef4672556800e4ac799ae835632bdba716209c5ca8c37433a6883dab4eb7cd67c1
24bc46c83b cli: Add warning for duplicate port definition (tdb3)
e208fb5d3b cli: Sanitize ports in rpcconnect and rpcport (tdb3)
Pull request description:
Adds invalid port detection to bitcoin-cli for -rpcconnect and -rpcport.
In addition to detecting malformed/invalid ports (e.g. those outside of the 16-bit port range, not numbers, etc.), bitcoin-cli also now considers usage of port 0 to be invalid. bitcoin-cli previously considered port 0 to be valid and attempted to use it to reach bitcoind.
Functional tests were added for invalid port detection as well as port prioritization.
Additionally, a warning is provided when a port is specified in both -rpcconnect and -rpcport.
This PR is an alternate approach to PR #27820 (e.g. SplitHostPort is unmodified).
Considered an alternative to 127.0.0.1 being specified in functional tests, but at first glance, this might need an update to test_framework/util.py (e.g. rpc_url), which might be left to a future PR.
ACKs for top commit:
S3RK:
light code review ACK 24bc46c83b
achow101:
ACK 24bc46c83b
cbergqvist:
re ACK 24bc46c83b
Tree-SHA512: c83ab6a30a08dd1ac8b368a7dcc2b4f23170f0b61dd67ffcad7bcda05096d333bcb9821fba11018151f55b2929c0a333bfec15b8bb863d83f41fc1974c6efca5
ab98e6fd03 test: add coverage for errors for `combinerawtransaction` RPC (brunoerg)
Pull request description:
This PR adds test coverage for the following errors for the `combinerawtransaction` RPC:
* Tx decode failed
* Missing transactions
* Input not found or already spent
For reference: https://maflcko.github.io/b-c-cov/total.coverage/src/rpc/rawtransaction.cpp.gcov.html
ACKs for top commit:
maflcko:
lgtm ACK ab98e6fd03
tdb3:
ACK ab98e6fd03
Tree-SHA512: 8a133c25dad2e1b236e0278a88796f60f763e3fd6fbbc080f926bb23f9dcc55599aa242d6e0c4ec15a179d9ded10a1f17ee5b6063719107ea84e6099f10416b2
0000276b31 test: Remove redundant verack check (MarcoFalke)
Pull request description:
Currently the sync in `connect_nodes` mentions the `version` and `verack` message types, but only checks the `verack`. Neither check is required, as the `pong` check implies both. In case of failure, the debug log will have to be consulted anyway, so the redundant check doesn't add value.
Also clarify in the comments that the goal is to check the flag `fSuccessfullyConnected` indirectly.
ACKs for top commit:
furszy:
utACK 0000276b31
brunoerg:
ACK 0000276b31
tdb3:
ACK 0000276b31
Tree-SHA512: f9ddcb1436d2f70da462a8dd470ecfc90a534dd6507c23877ef7626e7c02326c077001a42ad0171a87fba5c5275d1970d8c5e5d82c56c8412de944856fdfd6db
39d135e79f test: MiniWallet: respect fee_rate for target_weight, use in mempool_limit.py (Sebastian Falbesoner)
b2f0a9f8b0 test: add framework functional test for MiniWallet's tx padding (Sebastian Falbesoner)
c17550bc3a test: MiniWallet: fix tx padding (`target_weight`) for large sizes, improve accuracy (Sebastian Falbesoner)
Pull request description:
MiniWallet allows to create padded transactions that are equal or slightly above a certain `target_weight` (first introduced in PR #25379, commit 1d6b438ef0), which can be useful especially for mempool-related tests, e.g. for policy limit checks or scenarios to trigger mempool eviction. Currently the `target_weight` parameter doesn't play together with `fee_rate` though, as the fee calculation is incorrectly based on the tx vsize before the padding output is added, so the fee-rate is consequently far off. This means users are forced to pass an absolute fee, which can be quite inconvenient and leads to lots of duplicated "calculate absolute fee from fee-rate and vsize" code with the pattern `fee = (feerate / 1000) * (weight // 4)` on the call-sites.
This PR first improves the tx padding itself to be more accurate, adds a functional test for it, and fixes the `fee_rate` treatment for the `{create,send}_self_transfer` methods. (Next step would be to enable this also for the `_self_transfer_multi` methods, but those currently don't even offer a `fee_rate` parameter). Finally, the ability to pass both `target_weight` and `fee_rate` is used in the `mempool_limit.py` functional test. There might be more use-cases in other tests, that could be done in a follow-up.
ACKs for top commit:
rkrux:
tACK [39d135e](39d135e79f)
ismaelsadeeq:
Code Review ACK 39d135e79f🚀
glozow:
light review ACK 39d135e79f
Tree-SHA512: 6bf8e853a921576d463291d619cdfd6a7e74cf92f61933a563800ac0b3c023a06569b581243166906f56b3c5e8858fec2d8a6910d55899e904221f847eb0953d
f68cba29b3 blockman: Replace m_reindexing with m_blockfiles_indexed (Ryan Ofsky)
1b1c6dcca0 test: Add functional test for continuing a reindex (TheCharlatan)
201c1a9282 indexes: Don't wipe indexes again when already reindexing (TheCharlatan)
804f09dfa1 kernel: Add less confusing reindex options (Ryan Ofsky)
e172553223 validation: Remove needs_init from LoadBlockIndex (TheCharlatan)
533eab7d67 bugfix: Streamline setting reindex option (TheCharlatan)
Pull request description:
When restarting `bitcoind` during an ongoing reindex without setting the `-reindex` flag again, the block and coins db is left intact, but any data from the optional indexes is discarded. While not a bug per se, wiping the data again is
wasteful, both in terms of having to write it again, as well as potentially leading to longer startup times. So keep the index data instead when continuing a prior reindex.
Also includes a bugfix and smaller code cleanups around the reindexing code. The bug was introduced in b47bd95920: "kernel: De-globalize fReindex".
ACKs for top commit:
stickies-v:
ACK f68cba29b3
fjahr:
Code review ACK f68cba29b3
furszy:
Code review ACK f68cba29b3
ryanofsky:
Code review ACK f68cba29b3. Only changes since last review were cherry-picking suggested commits that rename variables, improving comments, and making some tweaks to test code.
Tree-SHA512: b252228cc76e9f1eaac56d5bd9e4eac23408e0fc04aeffd97a85417f046229364673ee1ca7410b9b6e7b692b03f13ece17c42a10176da0d7e975a8915deb98ca
This adds a bitset module that implements a BitSet<N> class, a variant
of std::bitset with a few additional features that cannot be implemented
in a wrapper without performance loss (specifically, finding first and
last bit set, or iterating over all set bits).
1f6ab1215b minor: remove unnecessary semicolons from RPC content type examples (Matthew Zipkin)
b225295298 test: use json-rpc 2.0 in all functional tests by default (Matthew Zipkin)
391843b029 bitcoin-cli: use json-rpc 2.0 (Matthew Zipkin)
d39bdf3397 test: remove unused variable in interface_rpc.py (Matthew Zipkin)
0ead71df8c doc: update and link for JSON-RPC 2.0 (Matthew Zipkin)
Pull request description:
This is a follow-up to #27101.
- Addresses [post-merge comments ](https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1606723428)
- bitcoin-cli uses JSON-RPC 2.0
- functional tests use JSON-RPC 2.0 by default (exceptions are in the regression tests added by #27101)
ACKs for top commit:
tdb3:
ACK 1f6ab1215b
cbergqvist:
ACK 1f6ab1215b
Tree-SHA512: 49bf14c70464081280216ece538a2f5ec810bac80a86a83ad3284f0f1b017edf755a1a74a45be279effe00218170cafde7c2de58aed07097a95c2c6b837a6b6c
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.
30a01134cd [doc] update bips.md for 431 (glozow)
9dbe6a03f0 [test] wallet uses CURRENT_VERSION which is 2 (glozow)
539404fe0f [policy] make v3 transactions standard (glozow)
052ede75af [refactor] use TRUC_VERSION in place of 3 (glozow)
Pull request description:
Make `nVersion=3` (which is currently nonstandard on mainnet) standard.
Note that we will treat these transactions as Topologically Restricted Until Confirmation (TRUC). Spec is in BIP 431 and implementation is in #28948, #29306, and #29873
See #27463 for overall project tracking, and #29319 for information about relevance to cluster mempool.
ACKs for top commit:
sdaftuar:
utACK 30a01134c
achow101:
ACK 30a01134cd
instagibbs:
utACK 30a01134cd
murchandamus:
ACK 30a01134cd
ismaelsadeeq:
ACK 30a01134cd🛰️
Tree-SHA512: 2a4aec0442c860e792a061d83e36483c1f1b426f946efbdf664c8db97a596e498b535707e1d3a900218429486ea69fd4552e3d476526a6883cbd5556c6534b48
fa52e13ee8 test: Remove struct.pack from almost all places (MarcoFalke)
fa826db477 scripted-diff: test: Use int.to_bytes over struct packing (MarcoFalke)
faf2a975ad test: Use int.to_bytes over struct packing (MarcoFalke)
faf3cd659a test: Normalize struct.pack format (MarcoFalke)
Pull request description:
`struct.pack` has many issues:
* The format string consists of characters that may be confusing and may need to be looked up in the documentation, as opposed to using easy to understand self-documenting code.
This lead to many test bugs, which weren't hit, which is fine, but still confusing. Ref: https://github.com/bitcoin/bitcoin/pull/29400, https://github.com/bitcoin/bitcoin/pull/29399, https://github.com/bitcoin/bitcoin/pull/29363, fa3886b7c6, ...
Fix all issues by using the built-in `int` helpers `to_bytes` via a scripted diff.
Review notes:
* For `struct.pack` and `int.to_bytes` the error behavior is the same, although the error messages are not identical.
* Two `struct.pack` remain. One for float serialization in a C++ code comment, and one for native serialization.
ACKs for top commit:
achow101:
ACK fa52e13ee8
rkrux:
tACK [fa52e13](fa52e13ee8)
theStack:
Code-review ACK fa52e13ee8
Tree-SHA512: ee80d935b68ae43d1654b047e84ceb80abbd20306df35cca848b3f7874634b518560ddcbc7e714e2e7a19241e153dee64556dc4701287ae811e26e4f8c57fe3e
The helper assumes that the n and k values have to be provided as a
single byte push operation, which is only possible for values up to 16.
Fix that by passing the numbers directly to the CScript list, where it's
automatically converted to minimally-encoded pushes (see class
method `CScript.__coerce_instance`, branch `isinstance(other, int)`).
In case of 17..20, this means that the data-pushes are done with two
bytes using OP_PUSH1 (0x01), e.g. for n=20: 0x01,0x14
4444de152f test: Set mocktime in p2p_disconnect_ban.py to avoid intermittent test failure (MarcoFalke)
fa6aa4027c test: Fix typos and use names args (MarcoFalke)
Pull request description:
Otherwise, the test may fail on slow hardware when running in valgrind.
Also, use named args for the absolute timepoint, while touching this file.
ACKs for top commit:
tdb3:
ACK for 4444de152f
AngusP:
re-ACK 4444de152f
Tree-SHA512: 660269c8dd18887d69b284f38656899caf028159ce83ddf921f3e9c080a5d0e663989f0e42b4baf4c4939f20f34da0e7e844dff9b7c91d0cab570c60958bd0e1
07aba8dd21 functional test: ensure confirmed utxo being sourced for 2nd chain (Greg Sanders)
Pull request description:
The test could fail/stop testing what we want if non-confirmed utxos become sourced through some internal change to `MiniWallet`; better to just fetch confirmed explicitly.
ACKs for top commit:
achow101:
ACK 07aba8dd21
ismaelsadeeq:
utACK 07aba8dd21
theStack:
ACK 07aba8dd21
Tree-SHA512: 66795fdf881139ed91bde0f8239a46bd9bc70bb311fa97c0e2b5537e1fd2a1fd36bf3a225fc77b9695deb835a9d6d29879aa1e05ea5054b9a33a400e199da014
2451a217dd test: addmultisigaddress, coverage for script size limits (furszy)
53302a0981 bugfix: addmultisigaddress, add unsupported operation for redeem scripts over 520 bytes (furszy)
9be6065cc0 test: coverage for 16-20 segwit multisig scripts (furszy)
9d9a91c4ea rpc: bugfix, incorrect segwit redeem script size used in signrawtransactionwithkey (furszy)
0c9fedfc45 fix incorrect multisig redeem script size limit for segwit (furszy)
f7a173b578 test: rpc_createmultisig, decouple 'test_sortedmulti_descriptors_bip67' (furszy)
4f33dbd8f8 test: rpc_createmultisig, decouple 'test_mixing_uncompressed_and_compressed_keys' (furszy)
25a81705d3 test: rpc_createmultisig, remove unnecessary checkbalances() (furszy)
b5a3289433 test: refactor, multiple cleanups in rpc_createmultisig.py (furszy)
3635d43268 test: rpc_createmultisig, remove manual wallet initialization (furszy)
Pull request description:
Fixing https://github.com/bitcoin/bitcoin/issues/28250#issuecomment-1674830104 and more.
Currently, redeem scripts longer than 520 bytes, which are technically valid under segwit rules, have flaws in the following processes:
1) The multisig creation process fails to deduce the output descriptor, resulting in the generation of an incorrect descriptor. Additionally, the accompanying user warning is also inaccurate.
2) The `signrawtransactionwithkey` RPC command fail to sign them.
3) The legacy wallet `addmultisigaddress` wrongly discards them.
The issue arises because most of these flows are utilizing the legacy spkm keystore, which imposes
the [p2sh max redeem script size rule](ded6873340/src/script/signingprovider.cpp (L160)) on all scripts. Which blocks segwit redeem scripts longer than
the max element size in all the previously mentioned processes (`createmultisig`, `addmultisigaddress`, and
`signrawtransactionwithkey`).
This PR fixes the problem, enabling the creation of multisig output descriptors involving more than 15 keys and
allowing the signing of these scripts, along with other post-segwit redeem scripts that surpass the 520-byte
p2sh limit.
Important note:
Instead of adding support for these longer redeem scripts in the legacy wallet, an "unsupported operation"
error has been added. The reasons behind this decision are:
1) The introduction of this feature brings about a compatibility-breaking change that requires downgrade
protection; older wallets would be unable to interact with these "new" legacy wallets.
2) Considering the ongoing deprecation of the legacy spkm, this issue provides another compelling
reason to transition towards descriptors.
Testing notes:
To easily verify each of the fixes, I decoupled the tests into standalone commits. So they can be
cherry-picked on top of master. Where `rpc_createmultisig.py` (with and without the `--legacy-wallet`
arg) will fail without the bugs fixes commits.
Extra note:
The initial commits improves the `rpc_createmultisig.py` test in many ways. I found this test very
antiquated, screaming for an update and cleanup.
ACKs for top commit:
pinheadmz:
ACK 2451a217dd
theStack:
Code-review ACK 2451a217dd
achow101:
ACK 2451a217dd
Tree-SHA512: 71794533cbd46b3a1079fb4e9d190d3ea3b615de0cbfa443466e14f05e4616ca90e12ce2bf07113515ea8113e64a560ad572bb9ea9d4835b6fb67b6ae596167f
df6dc2aaae test: Assumeutxo: snapshots with less work should not be loaded (Hernan Marino)
Pull request description:
This PR adds a test which checks that snapshots with less accumulated work than the node's active chain, should not be loaded and return with an error. Although in a different context of discussion the missing test was detect in a thread in https://github.com/bitcoin/bitcoin/pull/29394 (see https://github.com/bitcoin/bitcoin/pull/29394#discussion_r1484122214)
ACKs for top commit:
maflcko:
utACK df6dc2aaae
kevkevinpal:
utACK [df6dc2a](df6dc2aaae)
achow101:
ACK df6dc2aaae
alfonsoromanz:
Re ACK df6dc2aaae. Make is successful and the test passes.
Tree-SHA512: 07a394b4b288cc8ad3f66ed4e70dcda468db18113e9442eb7215cf491768432d55efaaa5b79d633094917e05475a30f0c5e4f64f8f2da293ba306891b4485560
71aae72e1f test: test sendall does ancestor aware funding (ishaanam)
36757941a0 wallet, rpc: implement ancestor aware funding for sendall (ishaanam)
544131f3fb rpc, test: test sendall spends unconfirmed change and unconfirmed inputs when specified (ishaanam)
Pull request description:
This PR:
- Adds a functional test that `sendall` spends unconfirmed change
- Adds a functional test that `sendall` spends regular unconfirmed inputs when specified by user
- Adds ancestor aware funding to `sendall` by using `calculateCombinedBumpFee` and adjusting the effective value accordingly
- Adds a functional test for ancestor aware funding in `sendall`
ACKs for top commit:
S3RK:
ACK 71aae72e1f
achow101:
ACK 71aae72e1f
furszy:
ACK 71aae72e1f
Tree-SHA512: acaeb7c65166ce53123a1d6cb5012197202246acc02ef9f37a28154cc93afdbd77c25e840ab79bdc7e0b88904014a43ab1ddea79d5337dc310ea210634ab61f0
