fafd9165e9 test: Add missing sync_all to feature_coinstatsindex (MarcoFalke)
Pull request description:
Sync the blocks before invalidating them to ensure all nodes are on the right tip. Otherwise nodes[0] might stay on the "stale" block and the test fails (intermittently)
ACKs for top commit:
jamesob:
crACK fafd9165e9
Tree-SHA512: ca567b97b839b56c91d52831eaac18d8c843d376be90c9fd8b49d2eb4a46b801a1d2402996d5dfe2bef3e2c9bd75d19ed443e3f42cc4679c5f20043ba556efc8
Adds an error output parameter to all GetReservedDestination functions
so that callers can get the actual reason that a change address could
not be fetched. This more closely matches GetNewDestination. This allows
for more granular error messages, such as one that indicates that
bech32m addresses cannot be generated yet.
We don't want the legacy wallet to ever have bech32m addresses so don't
allow importing them. This includes addmultisigaddress as that is a
legacy wallet only RPC
Additionally, bech32m multisigs are not available yet, so disallow them
in createmultisig.
The `on_addr` functionality of `AddrReceiver` tests logic specific to how the
addr messages are set up in the test bodies. To allow other callers to also use
`AddrReceiver`, only apply the assertion logic if the caller indicates
desirability by setting `test_addr_contents` to true when initializing the
class.
testshell in jupyter was failing due to an extra arg.
this adds a dummy -f param, which allows TestShell to
be used in a command line or jupyter environment
d6d2ab9845 test: MiniWallet: fix fee calculation for P2PK and check tx vsize (Sebastian Falbesoner)
ce024b1c0e test: MiniWallet: force P2PK signature to have fixed size (71 bytes) (Sebastian Falbesoner)
Pull request description:
This PR is a follow-up to #21945. It aims to both fix the fee calculation for P2PK mode transactions and enable its vsize check. Currently, the latter assumes a fixed tx length, which is fine for anyone-can-spend txs but doesn't apply to P2PK output spends due to varying DER signature size; the vsize check is therefore disabled for P2PK mode on master branch.
Creating one million DER signatures with MiniWallet shows the following distribution of sizes (smart people with better math skills probably could deduce the ratios without trying, but hey):
| DER signature size [bytes] | #occurences (ratio) |
| ------------- | ------------- |
| 71 | 498893 (49.89%) |
| 70 | 497244 (49.72%) |
| 69 | 3837 (0.38%) |
| 68 | 22 (0.0022%) |
Note that even smaller signatures are possible (for smaller R and S values with leading zero bytes), it's just that the probability decreases exponentially. Instead of choosing a large vsize check range and hoping that smaller signatures are never created (potentially leading to flaky tests), the proposed solution is ~~to limit the signature size to the two most common sizes 71 and 70 (>99.6% probability) and then accordingly only check for two vsize values; the value to be used for fee calculation is a decimal right between the two possible sizes (167.5 vbytes) and for the vsize check it's rounded down/up integer values are used.~~ to simply grind the signature to a fixed size of 71 bytes (49.89% probability, i.e. on average each call to `sign_tx()`, on average two ECC signing operations are needed).
~~The idea of grinding signatures to a fixed size (similar to https://github.com/bitcoin/bitcoin/pull/13666 which grinds to low-R values) would be counter-productive, as the signature creation in the test suite is quite expensive and this would significantly slow down tests that calculate hundreds of signatures (like e.g. feature_csv_activation.py).~~
For more about transaction sizes on different input/output types, see the following interesting article: https://medium.com/coinmonks/on-bitcoin-transaction-sizes-97e31bc9d816
ACKs for top commit:
MarcoFalke:
Concept ACK d6d2ab9845
Tree-SHA512: 011c70ee0e4adf9ba12902e4b6c411db9ae96bdd8bc810bf1d67713367998e28ea328394503371fc1f5087a819547ddaea56c073b28db893ae1c0031d7927f32
b9e76f1bf0 rpc: Add test for -rpcwaittimeout (Christian Decker)
f76cb10d7d rpc: Prefix rpcwaittimeout error with details on its nature (Christian Decker)
c490e17ef6 doc: Add release notes for the `-rpcwaittimeout` cli parameter (Christian Decker)
a7fcc8eb59 rpc: Add a `-rpcwaittimeout` parameter to limit time spent waiting (Christian Decker)
Pull request description:
Adds a new numeric `-rpcwaittimeout` that can be used to limit the
time we spend waiting on the RPC server to appear. This is used by
downstream projects to provide a bit of slack when `bitcoind`s RPC
interface is not available right away.
This makes the `-rpcwait` argument more useful, since we can now limit
how long we'll ultimately wait, before potentially giving up and reporting
an error to the caller. It was discussed in the context of the BTCPayServer
wanting to have c-lightning wait for the RPC interface to become available
but still have the option of giving up eventually ([4355]).
I checked with laanwj whether this is already possible ([comment]), and
whether this would be a welcome change. Initially I intended to repurpose
the (optional) argument to `-rpcwait`, however I decided against it since it
would potentially break existing configurations, using things like `rpcwait=1`,
or `rpcwait=true` (the former would have an unintended short timeout, when
old behavior was to wait indefinitely).
~Due to its simplicity I didn't implement a test for it yet, but if that's desired I
can provide one.~ Test was added during reviews.
[4355]: https://github.com/ElementsProject/lightning/issues/4355
[comment]: https://github.com/ElementsProject/lightning/issues/4355#issuecomment-768288261
ACKs for top commit:
laanwj:
Code review ACK b9e76f1bf0
promag:
ACK b9e76f1bf0.
Tree-SHA512: 3cd6728038ec7ca7c35c2e7ccb213bfbe963f99a49bb48bbc1e511c4dd23d9957c04f9af1f8ec57120e47b26eaf580b46817b099d5fc5083c98da7aa92db8638
With `banlist.dat` (being written in addrv1 format) if we would try to
write a Tor v3 subnet, it would serialize as a dummy-all-0s IPv6
address and subsequently, when deserialized will not result in the same
subnet.
This problem does not exist with `banlist.json` where the data is saved
in textual, human-readable form.
30aee2dfe6 tests: Add test for compact block HB selection (Pieter Wuille)
6efbcec4de Protect last outbound HB compact block peer (Suhas Daftuar)
Pull request description:
If all our high-bandwidth compact block serving peers (BIP 152) stall block
download, then we can be denied a block for (potentially) a long time. As
inbound connections are much more likely to be adversarial than outbound
connections, mitigate this risk by never removing our last outbound HB peer if
it would be replaced by an inbound.
ACKs for top commit:
achow101:
ACK 30aee2dfe6
ariard:
Code ACK 30aee2dfe
jonatack:
ACK 30aee2dfe6
Tree-SHA512: 5c6c9326e3667b97e0864c371ae2174d2be9054dad479f4366127b9cd3ac60ffa01ec9707b16ef29cac122db6916cf56fd9985733390017134ace483278921d5
fa7d71f270 test: Run pep-8 on touched test (MarcoFalke)
fab7e99c2a test: Use MiniWallet in test_no_inherited_signaling RBF test (MarcoFalke)
fab871f649 test: Remove unused generate() from test (MarcoFalke)
faff3f35b7 test: Add txin.sequence option to MiniWallet (MarcoFalke)
Pull request description:
This comes with nice benefits:
* Less code and complexity
* Test can be run without wallet compiled in
Also add some additional checks for `getmempoolentry` (#22209) and other cleanups 🎨
ACKs for top commit:
mjdietzx:
Tested ACK fa7d71f270 thanks for the explanations, nicely done
theStack:
ACK fa7d71f270🍷
Tree-SHA512: 0e9b8fe985779d8d7034d256deed627125bb374b6ae2972c461b3a220739a51061c6147ad69339bee16282f82716c7f3f8a7a89c693ceb1e47ea50709272332a
55311197c4 Added new test for future blocks reacceptance (sanket1729)
511a5af462 Fixed inconsistencies between code and comments (sanket1729)
Pull request description:
This Commit does 3 things:
1) Adds a test case for checking reacceptance a previously rejected block which
was too far in the future.
~~2) clean up uses of rehash or calc_sha256 where it was not needed~~
3) While constructing block 44, this commit makes the code consistent with the expected figure in
the comment just above it by adding a transaction to the block.
4) Fix comment describing `sign_tx()` function
ACKs for top commit:
duncandean:
reACK 5531119
brunoerg:
reACK 55311197c4
Tree-SHA512: d40c72fcdbb0b2a0715adc58441eeea08147ee2ec5e371a4ccc824ebfdc6450698bd40aaeecb7ea7bfdb3cd1b264dd821b890276fff8b8d89b7225cdd9d6b546
451b96f7d2 test: kill process group to avoid dangling processes (S3RK)
Pull request description:
This is an alternative to #19281
This PR fixes a problem when after test failure with `--failfast` option there could be dangling nodes. The nodes will continue to occupy rpc/p2p ports on the machine and will cause further test failures.
If there are any dangling nodes left at the end of the test run we kill the whole process group.
Pros: the operations is immediate and won't lead to CI timeout
Cons: the test_runner process is also killed and exit code is 137
Example output:
```
...
Early exiting after test failure
TEST | STATUS | DURATION
rpc_decodescript.py | ✓ Passed | 2 s
rpc_deprecated.py | ✓ Passed | 2 s
rpc_deriveaddresses.py | ✓ Passed | 2 s
rpc_dumptxoutset.py | ✖ Failed | 2 s
ALL | ✖ Failed | 8 s (accumulated)
Runtime: 4 s
Killed: 9
> echo $?
137
```
ACKs for top commit:
MarcoFalke:
review ACK 451b96f7d2
aitorjs:
ACK 451b96f7d2. Manual testing with and without **--failfast**.
Tree-SHA512: 87e510a1411b9e7571e63cf7ffc8b9a8935daf9112ffc0f069d6c406ba87743ec439808181f7e13cb97bb200fad528589786c47f0b43cf3a2ef0d06a23cb86dd
458a345b05 Add support for SIGHASH_DEFAULT in RPCs, and make it default (Pieter Wuille)
c0f0c8eccb tests: check spending of P2TR (Pieter Wuille)
a2380127e9 Basic Taproot signing logic in script/sign.cpp (Pieter Wuille)
49487bc3b6 Make GetInputUTXO safer: verify non-witness UTXO match (Pieter Wuille)
fd3f6890f3 Construct and use PrecomputedTransactionData in PSBT signing (Pieter Wuille)
5cb6502ac5 Construct and use PrecomputedTransactionData in SignTransaction (Pieter Wuille)
5d2e22437b Don't nuke witness data when signing fails (Pieter Wuille)
ce9353164b Permit full precomputation in PrecomputedTransactionData (Pieter Wuille)
e841fb503d Add precomputed txdata support to MutableTransactionSignatureCreator (Pieter Wuille)
a91d532338 Add CKey::SignSchnorr function for BIP 340/341 signing (Pieter Wuille)
e77a2839b5 Use HandleMissingData also in CheckSchnorrSignature (Pieter Wuille)
dbb0ce9fbf Add TaprootSpendData data structure, equivalent to script map for P2[W]SH (Pieter Wuille)
Pull request description:
Builds on top of #22051, adding signing support after derivation support.
Nothing is changed in descriptor features. Signing works for key path and script path spending, through the normal sending functions, and PSBT-based RPCs. However, PSBT usability is rather low as no extensions have been defined to convey Taproot-specific information, so all script information must be known to the signing wallet.
ACKs for top commit:
achow101:
re-ACK 458a345b05
fjahr:
Code review ACK 458a345b05
Sjors:
ACK 458a345b05
Tree-SHA512: 30ed212cf7754763a4a81624ebc084c51727b8322711ac0b390369213c1a891d367ed8b123882ac08c99595320c11ec57ee42304ff22a69afdc3d1a0d55cc711
754e802274 test: check rejected future block later accepted (Luke Dashjr)
Pull request description:
(Luke) was unsure if the code sufficiently avoided caching a
time-too-new rejection, so wrote this test to check it. It looks like
despite only exempting BLOCK_MUTATED, it is still okay because header
failures never cache block invalidity. This test will help ensure that
if this ever changes, BLOCK_TIME_FUTURE gets excluded at the same time.
This PR re-opens https://github.com/bitcoin/bitcoin/pull/17872 which went stale and addresses the nits raised by reviewers there.
ACKs for top commit:
MarcoFalke:
review ACK 754e802274
Tree-SHA512: a2bbc8fffb523cf2831e1ecb05f20868e30106a38cc2e369e4973fa549cca06675a668df16f76c49cc4ce3a22925404255e5c53c4232d63ba1b9fca878509aa0
1) Makes the code for block 44 consistent with the expected figure in
the comment above it by adding a transaction to the block
2) Fixed comment describing sign_tx() function
(Luke) was unsure if the code sufficiently avoided caching a
time-too-new rejection, so wrote this test to check it. It looks like
despite only exempting BLOCK_MUTATED, it is still okay because header
failures never cache block invalidity. This test will help ensure that
if this ever changes, BLOCK_TIME_FUTURE gets excluded at the same time.
Co-authored-by: Will Clark <will8clark@gmail.com>
01eedf3821 test: doc: improve doc for chain_transaction() helper (Sebastian Falbesoner)
6e63e366d6 test: refactor: dedup utility function chain_transaction() (Sebastian Falbesoner)
Pull request description:
Both tests `mempool_packages.py` and `mempool_package_onemore.py` define a utility function `chain_transaction` with a similar implementation. This PR deduplicates it by moving it into the util package and keeping the more general properties:
* pass a list of parent_txids/vouts instead of single values
* always mark the BIP125-replaceable flag for txs, created via `createrawtransaction` (this is needed by the `mempool_package_onemore.py` test, but doesn't hurt the other one)
This is a low-hanging fruit; as a potential follow-up one could probably also deduplicate the function `chain_transaction` in `rpc_packages.py`, which looks a bit different, as it also takes the parent locking script into account and doesn't send the tx.
ACKs for top commit:
mjdietzx:
reACK 01eedf3821
klementtan:
Code review ACK 01eedf3821
MarcoFalke:
review ACK 01eedf3821🙅
Tree-SHA512: ac7105d02c23f53d76d4ec9dc8de1074dd8faefeecd44b107921b78665279498966152fed312ecbe252a1c34a9643d531166329a4fea0e773df3bb75d43092b0
fbf485c9b2 Allow tr() import only when Taproot is active (Andrew Chow)
Pull request description:
To avoid issues around fund loss, only allow descriptor wallets to import `tr()` descriptors after taproot has activated.
ACKs for top commit:
sipa:
utACK fbf485c9b2
fjahr:
Code review ACK fbf485c9b2
laanwj:
Code review ACK fbf485c9b2
prayank23:
utACK fbf485c9b2
Tree-SHA512: 83c43376515eea523dbc89bc5a0fde53e54aec492e49a40c2a33d80fc94aac459e232ae07b024b4bd75b58078c8d090bc7a2d69541c5d3d4834d2f4cfc9c8208
In order to enable exact fee calculation for transactions that spend
P2PK outputs in the MiniWallet, we enforce the created signatures to
have a fixed length (>49.89% probability) by default. With that it is
easier to check the created transactions vsize and avoid flaky tests
that would appear whenever the signatures R- or S-values are smaller
(due to leading zero bytes).
Note that to get the total scriptSig size one has to add another
2 bytes, as there is also the OP_PUSHx instruction on the front and
the sighash type byte on the back, leading to a final scriptSig size
of 73 bytes.
2eb0eeda39 validation: document lack of inherited signaling in RBF policy (Antoine Riard)
906b6d9da6 test: Extend feature_rbf.py with no inherited signaling (Antoine Riard)
Pull request description:
Contrary to BIP125 or other full-node implementation (e.g btcd), Bitcoin Core's mempool policy doesn't implement inherited signaling.
This PR documents our mempool behavior on this and add a test demonstrating the case.
ACKs for top commit:
jonatack:
ACK 2eb0eeda39
benthecarman:
ACK 2eb0eeda39
Tree-SHA512: d41453d3b49bae3c1eb532a968f43bc047084913bd285929d4d9cba142777ff2be38163d912e28dfc635f4ecf446de68effad799c6e71be52f81e83410c712fb
Add an explicit sync_blocks call in wallet_orphanedreward.py, which was
missing and could lead to intermittent failures of the test due to
race conditions.
This will presumably fix https://github.com/bitcoin/bitcoin/issues/22181.
faa94961d6 test: Add temporary logging to debug #20975 (MarcoFalke)
Pull request description:
to be reverted after a fix
ACKs for top commit:
laanwj:
Code review ACK faa94961d6
Tree-SHA512: 1f3103fcf4cad0af54e26c4d257bd824b128b5f2d2b81c302e861a829fd55d6a099fa476b79b30a71fe98975ae604b9e3ff31fd48a51d442389a9bd515e60ba0
68ace23fa3 test: convert docs into type annotations in test_framework/test_node.py (fanquake)
8bfcba36db test: convert docs into type annotations in test_framework/wallet.py (fanquake)
b043ca8e8b test: convert docs into type annotations in test_framework/util.py (fanquake)
Pull request description:
Rather than having function types exist as documentation, make them type annotations, which enables more `mypy` checking.
ACKs for top commit:
instagibbs:
utACK 68ace23fa3
Tree-SHA512: b705f26b48baabde07b9b2c0a8d547b4dcca291b16eaf5ac70462bb3a1f9e9c2783d93a9d4290889d0cbb3f7db3671446754411a1f498b265d336f6ff33478df
fa2b6c62cd test: Remove unused node from feature_nulldummy (MarcoFalke)
Pull request description:
This is confusing and might even slow down the test.
This reverts a change that was added a year ago in d438d60 and then the need for it was removed by 95d5d5e six months ago.
Top commit has no ACKs.
Tree-SHA512: 9a86792e9a634cf7bbd4e7a21b1acdfc3baba1b1962fe2b9b73848436d10351d2326dca01313c097ba2342dde7207add73e731d053c0bfa888a5d8f2b233a7cf
e4356f6a6c Testcase for wallet issue with orphaned rewards. (Daniel Kraft)
Pull request description:
This adds a new test case demonstrating the wallet issue when block rewards are orphaned (#14148).
ACKs for top commit:
LarryRuane:
ACK e4356f6a6c
leonardojobim:
reACK e4356f6a6c .
Tree-SHA512: e9a2310ee1b3d52cfa302f431ed3d272bbc1b9195439ff318d9eb1006c0b28968dbe840e1600b6ff185e5d7ea57e4dcc837cef16051b5537445e10bc363b8c22
2667366aaa tests: check derivation of P2TR (Pieter Wuille)
7cedafc541 Add tr() descriptor (derivation only, no signing) (Pieter Wuille)
90fcac365e Add TaprootBuilder class (Pieter Wuille)
5f6cc8daa8 Add XOnlyPubKey::CreateTapTweak (Pieter Wuille)
2fbfb1becb Make consensus checking of tweaks in pubkey.* Taproot-specific (Pieter Wuille)
a4bf84039c Separate WitnessV1Taproot variant in CTxDestination (Pieter Wuille)
41839bdb89 Avoid dependence on CTxDestination index order (Pieter Wuille)
31df02a070 Change Solver() output for WITNESS_V1_TAPROOT (Pieter Wuille)
4b1cc08f9f Make XOnlyPubKey act like byte container (Pieter Wuille)
Pull request description:
This is a subset of #21365, to aide review.
This adds support `tr(KEY)` or `tr(KEY,SCRIPT)` or `tr(KEY,{{S1,{{S2,S3},...}},...})` descriptors, describing Taproot outputs with specified internal key, and optionally any number of scripts, in nested groups of 2 inside `{`/`}` if there are more than one. While it permits importing `tr(KEY)`, anything beyond that is just laying foundations for more features later.
Missing:
* Signing support (see #21365)
* Support for more interesting scripts inside the tree (only `pk(KEY)` is supported for now). In particular, a multisig policy based on the new `OP_CHECKSIGADD` opcode would be very useful.
* Inferring `tr()` descriptors from outputs (given sufficient information).
* `getaddressinfo` support.
* MuSig support. Standardizing that is still an ongoing effort, and is generally kind of useless without corresponding PSBT support.
* Convenient ways of constructing descriptors without spendable internal key (especially ones that arent't trivially recognizable as such).
ACKs for top commit:
Sjors:
utACK 2667366 (based on https://github.com/bitcoin/bitcoin/pull/21365#issuecomment-846945215 review, plus the new functional test)
achow101:
Code Review ACK 2667366aaa
lsilva01:
Tested ACK 2667366aaa
meshcollider:
utACK 2667366aaa
Tree-SHA512: 61046fef22c561228338cb178422f0b782ef6587ec8208d3ce2bd07afcff29a664b54b35c6b01226eb70b6540b43f6dd245043d09aa6cb6db1381b6042667e75
5d82a57db4 contrib: remove torv2 seed nodes (Jon Atack)
5f7e086dac contrib: update generate-seeds.py to ignore torv2 addresses (Jon Atack)
8be56f0f8e p2p, refactor: extract OnionToString() from CNetAddr::ToStringIp() (Jon Atack)
5f9d3c09b4 p2p: remove torv2 from CNetAddr::ToStringIP() (Jon Atack)
3d39042144 p2p: remove torv2 in SetIP() and ADDR_TORV2_SIZE constant (Jon Atack)
cff5ec477a p2p: remove pre-addrv2 onions from SerializeV1Array() (Jon Atack)
4192a74413 p2p: ignore torv2-in-ipv6 addresses in SetLegacyIPv6() (Jon Atack)
1d631e956f p2p: remove BIP155Network::TORV2 from GetBIP155Network() (Jon Atack)
7d1769bc45 p2p: remove torv2 from SetNetFromBIP155Network() (Jon Atack)
eba9a94b9f fuzz: rename CNetAddr/CService deserialize targets (Jon Atack)
c56a1c9b18 p2p: drop onions from IsAddrV1Compatible(), no longer relay torv2 (Jon Atack)
f8e94002fc p2p: remove torv2/ADDR_TORV2_SIZE from SetTor() (Jon Atack)
0f1c58ae87 test: update feature_proxy to torv3 (Jon Atack)
Pull request description:
This patch removes support in Bitcoin Core for Tor v2 onions, which are already removed from the release of Tor 0.4.6.
- no longer serialize/deserialize and relay Tor v2 addresses
- ignore incoming Tor v2 addresses
- remove Tor v2 addresses from the addrman and peers.dat on node launch
- update generate-seeds.py to ignore Tor v2 addresses
- remove Tor v2 hard-coded seeds
Tested with tor-0.4.6.1-alpha (no v2 support) and 0.4.5.7 (v2 support). With the latest Tor (no v2 support), this removes all the warnings like those reported with current master in https://github.com/bitcoin/bitcoin/issues/21351
```
<bitcoind debug log>
Socks5() connect to […].onion:8333 failed: general failure
<tor log>
Invalid hostname [scrubbed]; rejecting
```
and the addrman no longer has Tor v2 addresses on launching bitcoind.
```rake
$ ./src/bitcoin-cli -addrinfo
{
"addresses_known": {
"ipv4": 44483,
"ipv6": 8467,
"torv2": 0,
"torv3": 2296,
"i2p": 6,
"total": 55252
}
}
```
After recompiling back to current master and restarting with either of the two Tor versions (0.4.5.7 or 0.4.6.1), -addrinfo initially returns 0 Tor v2 addresses and then begins finding them again.
Ran nodes on this patch over the past week on mainnet/testnet/signet/regtest after building with DEBUG_ADDRMAN.
Verified that this patch bootstraps an onlynet=onion node from the Tor v3 hardcoded fixed seeds on mainnet and testnet and connects to blocks and v3 onion peers: `rm ~/.bitcoin/testnet3/peers.dat ; ./src/bitcoind -testnet -dnsseed=0 -onlynet=onion`
Tested using `addnode`, `getaddednodeinfo`,`addpeeraddress`, `disconnectnode` and `-addrinfo` that a currently valid, connectable Tor v2 peer can no longer be added:
Thanks to Vasil Dimov, Carl Dong, and Wladimir J. van der Laan for their work on BIP155 and Tor v3 that got us here.
ACKs for top commit:
laanwj:
Code review ACK 5d82a57db4
Tree-SHA512: 590ff3d2f6ef682608596facb4b01f44fef69716d2ab3552ae1655aa225f4bf104f9ee08d6769abb9982a8031de93340df553279ce1f5023771f9f2b651178bb
a3f0cbf82d test: run mempool_reorg.py even with wallet disabled (Darius Parvin)
Pull request description:
Run mempool_reorg.py test even when the wallet is disabled, as discussed in #20078.
As part of this PR I created a new method in `MiniWallet`, `create_self_transfer`, to return a raw tx (without broadcasting it) and its associated utxo.
ACKs for top commit:
MarcoFalke:
cr ACK a3f0cbf82d
Tree-SHA512: 316a38faffadcb87499c1d6eca21e9696cef65362bbffcf621788a9b771bb1fa2971b1c7835cbd34b952d7612ad83afbca824cd8be39ecd6b994e8963027f991
The functions cltv_modify_tx(), cltv_invalidate() and cltv_validate()
all modify the passed transaction in-place, i.e. there is no need
to return a copy.
- run mempool_reorg.py even when the wallet is not compiled
- add `locktime` argument to `create_self_transfer` and `send_self_transfer`
- use more logs instead of comments
bfa9309ad6 Use COINBASE_MATURITY constant in functional tests. (Kiminuo)
525448df9d Move COINBASE_MATURITY from `feature_nulldummy` test to `blocktools`. (Kiminuo)
Pull request description:
`COINBASE_MATURITY` constant was added to `feature_nulldummy` test in #21373. This PR moves the constant to `blocktools.py` file and uses the constant in more tests as suggested [here](https://github.com/bitcoin/bitcoin/pull/21373#discussion_r605418462).
Edit: Goal of this PR is to replace integer constants with `COINBASE_MATURITY` but not necessarily in *all* cases because that would mean to read and fully understand all tests. That's out of my time constraints. Any reports where `COINBASE_MATURITY` should be used are welcome though!
ACKs for top commit:
theStack:
ACK bfa9309ad6🌇
Tree-SHA512: 01f04645f05a39028681f355cf3d42dd63ea3303f76d93c430e0fdce441934358a2d847a54e6068d61932f1b75e1d406f51859b057b3e4b569f7083915cb317f
2be3572506 test: Fix IPv6 check on BSD systems (nthumann)
Pull request description:
I noticed that `test_ipv6_local()` always returns `False` on macOS or FreeBSD, even though IPv6 is working perfectly fine. This causes `test/functional/rpc_bind.py --ipv6` and `test/functional/feature_proxy.py` to skip their run.
Apparently, there's a check if the port number is `0` (see [here](64881da478/sys/netinet6/udp6_usrreq.c (L248)) or [here](8f02f2a044/bsd/netinet6/udp6_usrreq.c (L282))), while Linux has no problem with this.
This is fixed by specifying any other port number than `0`, e.g. `1`. Still, because of `SOCK_DGRAM`, no actual connection is made.
ACKs for top commit:
fanquake:
ACK 2be3572506 - nice improvement. I checked that with this change ipv6 related tests in `feature_proxy.py` are being run.
theStack:
ACK 2be3572506
Tree-SHA512: 8417c2d3cf71050529f3fa409a03872040fe5d249eae4172f276e62156e505a20d375b963712a186c9ad7967d8a497b5900d327c74a9693f68c33063871d4691
13650fe2e5 [policy] detect unsorted packages (glozow)
9ef643e21b [doc] add release note for package testmempoolaccept (glozow)
c4259f4b7e [test] functional test for packages in RPCs (glozow)
9ede34a6f2 [rpc] allow multiple txns in testmempoolaccept (glozow)
ae8e6df709 [policy] limit package sizes (glozow)
c9e1a26d1f [fuzz] add ProcessNewPackage call in tx_pool fuzzer (glozow)
363e3d916c [test] unit tests for ProcessNewPackage (glozow)
cd9a11ac96 [test] make submit optional in CreateValidMempoolTransaction (glozow)
2ef187941d [validation] package validation for test accepts (glozow)
578148ded6 [validation] explicit Success/Failure ctors for MempoolAcceptResult (glozow)
b88d77aec5 [policy] Define packages (glozow)
249f43f3cc [refactor] add option to disable RBF (glozow)
897e348f59 [coins/mempool] extend CCoinsViewMemPool to track temporary coins (glozow)
42cf8b25df [validation] make CheckSequenceLocks context-free (glozow)
Pull request description:
This PR enables validation dry-runs of packages through the `testmempoolaccept` RPC. The expectation is that the results returned from `testmempoolaccept` are what you'd get from test-then-submitting each transaction individually, in that order (this means the package is expected to be sorted in topological order, for now at least). The validation is also atomic: in the case of failure, it immediately halts and may return "unfinished" `MempoolAcceptResult`s for transactions that weren't fully validated. The API for 1 transaction stays the same.
**Motivation:**
- This allows you to test validity for transaction chains (e.g. with multiple spending paths and where you don't want to broadcast yet); closes#18480.
- It's also a first step towards package validation in a minimally invasive way.
- The RPC commit happens to close#21074 by clarifying the "allowed" key.
There are a few added restrictions on the packages, mostly to simplify the logic for areas that aren't critical to main package use cases:
- No package can have conflicts, i.e. none of them can spend the same inputs, even if it would be a valid BIP125 replacement.
- The package cannot conflict with the mempool, i.e. RBF is disabled.
- The total count of the package cannot exceed 25 (the default descendant count limit), and total size cannot exceed 101KvB (the default descendant size limit).
If you're looking for review comments and github isn't loading them, I have a gist compiling some topics of discussion [here](https://gist.github.com/glozow/c3acaf161c95bba491fce31585b2aaf7)
ACKs for top commit:
laanwj:
Code review re-ACK 13650fe2e5
jnewbery:
Code review ACK 13650fe2e5
ariard:
ACK 13650fe
Tree-SHA512: 8c5cbfa91a6c714e1c8710bb281d5ff1c5af36741872a7c5df6b24874d6272b4a09f816cb8a4c7de33ef8e1c2a2c252c0df5105b7802f70bc6ff821ed7cc1a2f
e6fe1c37d0 rpc: Improve avoidpartialspends and avoid_reuse documentation (Fabian Jahr)
8f073076b1 wallet: Increase OUTPUT_GROUP_MAX_ENTRIES to 100 (Fabian Jahr)
Pull request description:
Follow-up to #17824.
This increases OUTPUT_GROUP_MAX_ENTRIES to 100 which means that OutputGroups will now be up to 100 outputs large, up from previously 10. The main motivation for this change is that during the PR review club on #17824 [several participants signaled](https://bitcoincore.reviews/17824.html#l-339) that 100 might be a better value here.
I think fees should be manageable for users but more importantly, users should know what they can expect when using the wallet with this configuration, so I also tried to clarify the documentation on `-avoidpartialspends` and `avoid_reuse` a bit. If there are other additional ways how or docs where users can be made aware of the potential consequences of using these parameters, please let me know. Another small upside is that [there seem to be a high number of batching transactions with 100 and 200 inputs](https://miro.medium.com/max/3628/1*sZ5eaBSbsJsHx-J9iztq2g.png)([source](https://medium.com/@hasufly/an-analysis-of-batching-in-bitcoin-9bdf81a394e0)) giving these transactions a bit of a larger anonymity set, although that is probably a very weak argument.
ACKs for top commit:
jnewbery:
ACK e6fe1c37d0
Xekyo:
retACK e6fe1c37d0
rajarshimaitra:
tACK `e6fe1c3`
achow101:
ACK e6fe1c37d0
glozow:
code review ACK e6fe1c37d0
Tree-SHA512: 79685c58bafa64ed8303b0ecd616fce50fc9a2b758aa79833e4ad9f15760e09ab60c007bc16ab4cbc4222e644cfd154f1fa494b0f3a5d86faede7af33a6f2826
Only allow "packages" with no conflicts, sorted in order of dependency,
and no more than 25 for now. Note that these groups of transactions
don't necessarily need to adhere to some strict definition of a package
or have any dependency relationships. Clients are free to pass in a
batch of 25 unrelated transactions if they want to.
For the MiniWallet constructor, the two boolean parameters
"raw_script" and "use_p2pk" are replaced by a single parameter of the
newly introduced type MiniWalletMode (derived by enum.Enum), which can
hold the following values:
- ADDRESS_OP_TRUE
- RAW_OP_TRUE
- RAW_P2PK
4bea301692 test: use P2PK-MiniWallet for feature_csv_activation.py (Sebastian Falbesoner)
dc7eb64e83 test: MiniWallet: add P2PK support (Sebastian Falbesoner)
Pull request description:
This PR adds support for creating and spending transactions with raw pubkey (P2PK) outputs to MiniWallet, [as suggested by MarcoFalke](https://github.com/bitcoin/bitcoin/pull/21900#discussion_r629524841). Using that mode in the test `feature_csv_activation.py`, all txs submitted to the mempool follow the standard policy, i.e. `-acceptnonstdtxn=1` can be removed.
Possible follow-ups:
* Improve MiniWallet constructor Interface; an enum-like parameter instead of two booleans would probably be better
* Look at other tests that could benefit from P2PK (e.g. feature_cltv.py?)
* Check vsize also for P2PK txs (vsize varies due to signature, i.e. a range has to be asserted)
ACKs for top commit:
laanwj:
Code review ACK 4bea301692
Tree-SHA512: 9b428e6b7cfde59a8c7955d5096cea88af1384a5f49723f00052e9884d819d952d20a5ab39bb02f9d8b6073769c44462aa265d84a33e33da33c2d21670c488a6
