aboutsummaryrefslogtreecommitdiff
path: root/test/functional
AgeCommit message (Collapse)Author
2024-02-29Merge bitcoin/bitcoin#29390: test: speedup bip324_cipher.py unit testAva Chow
a8c3454ba137dfac20b3c89bc558374de0524114 test: speedup bip324_cipher.py unit test (Sebastian Falbesoner) Pull request description: Executing the unit tests for the bip324_cipher.py module currently takes quite long (>60 seconds on my older notebook). Most time here is spent in empty plaintext/ciphertext encryption/decryption loops in `test_fschacha20poly1305aead`: https://github.com/bitcoin/bitcoin/blob/9eeee7caa3f95ee17a645e12d330261f8e3c2dbf/test/functional/test_framework/crypto/bip324_cipher.py#L193-L194 https://github.com/bitcoin/bitcoin/blob/9eeee7caa3f95ee17a645e12d330261f8e3c2dbf/test/functional/test_framework/crypto/bip324_cipher.py#L198-L199 Their sole purpose is increasing the FSChaCha20Poly1305 packet counter in order to trigger rekeying, i.e. the actual encryption/decryption is not relevant, as the result is thrown away. This commit speeds up the tests by supporting to pass "None" as plaintext/ciphertext, indicating to the routines that no actual encryption/decryption should be done. The approach here is a bit hacky, a cleaner alternative would probably be to introduce a special `seek`/`skip_packets` method and not touch the encrypt/decrypt routines, but that seemed overkill to me only for speeding up a unit test. Open for suggestions. master branch: ``` $ python3 -m unittest ./test/functional/test_framework/crypto/bip324_cipher.py .. ---------------------------------------------------------------------- Ran 2 tests in 64.658s ``` PR branch: ``` $ python3 -m unittest ./test/functional/test_framework/crypto/bip324_cipher.py .. ---------------------------------------------------------------------- Ran 2 tests in 0.822s ``` ACKs for top commit: delta1: Concept ACK a8c3454 epiccurious: Tested ACK a8c3454ba137dfac20b3c89bc558374de0524114. achow101: ACK a8c3454ba137dfac20b3c89bc558374de0524114 marcofleon: ACK a8c3454ba137dfac20b3c89bc558374de0524114. The comments at the top of `bip324_cipher.py` specify that this should only be used for testing, so I think this optimization makes sense in that context. cbergqvist: ACK a8c3454! stratospher: ACK a8c3454. I think it's worth it because of the significant speedup in the unit test. Tree-SHA512: 737dd805a850be6e035aa3c6d9e2c5b5b5e89ddc564f84a045c37e0238fef6419912de7c902139b64914abdd647c649fe02a694f1a5e1741d7d4459c041caccc
2024-02-29Merge bitcoin/bitcoin#29510: wallet: `getrawchangeaddress` and ↵Ava Chow
`getnewaddress` failures should not affect keypools for descriptor wallets e073f1dfda7a2a2cb2be9fe2a1d576f122596021 test: make sure keypool sizes do not change on `getrawchangeaddress`/`getnewaddress` failures (UdjinM6) 367bb7a80cc71130995672c853d4a6e0134721d6 wallet: Avoid updating `ReserveDestination::nIndex` when `GetReservedDestination` fails (UdjinM6) Pull request description: I think the expected behaviour of `getrawchangeaddress` and `getnewaddress` RPCs is that their failure should not affect keypool in any way. At least that's how legacy wallets work, you can confirm this behaviour by running `wallet_keypool.py --legacy-wallet` on master with e073f1dfda7a2a2cb2be9fe2a1d576f122596021 applied on top. However running `wallet_keypool.py --descriptors` on the same commit results in the following failure: ``` File "/path/to/bitcoin/test/functional/test_framework/test_framework.py", line 131, in main self.run_test() File "/path/to/bitcoin/test/functional/wallet_keypool.py", line 114, in run_test assert_equal(kp_size_before, kp_size_after) File "/path/to/bitcoin/test/functional/test_framework/util.py", line 57, in assert_equal raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args)) AssertionError: not([18, 24] == [19, 24]) ``` This happens because we pass `nIndex` (which is a class member) into `GetReservedDestination` and since it's passed by reference we get an updated value back, so `nIndex` won't be equal `-1` anymore, no matter if the function failed or succeeded. This means that `ReturnDestination` (called by dtor of `ReserveDestination`) will try to return something we did not actually reserve. The fix is to simply use a temporary variable instead of a class member and only update `nIndex` when `op_address` actually has value, basically do it the same way we do for other class members (`address` and `fInternal`) already. ACKs for top commit: achow101: ACK e073f1dfda7a2a2cb2be9fe2a1d576f122596021 josibake: ACK https://github.com/bitcoin/bitcoin/pull/29510/commits/e073f1dfda7a2a2cb2be9fe2a1d576f122596021 Tree-SHA512: 1128288a60dd4d8f306ef6f7ac66cdfeae3c9cc35c66ecada2d78fa61ac759f2a757b70fc3976ba8b5081200942b58dfabc184c01ccf911af40ba8c145344651
2024-02-29Merge bitcoin/bitcoin#29511: test: Fix intermittent failure in rpc_net.py ↵Ava Chow
--v2transport 0487f91a2046c0d6f91ccaedeb00fbefa635c66d test: Fix intermittent failure in rpc_net.py --v2transport (stratospher) Pull request description: Fixes #29508. Make sure that v2 handshake is complete before comparing getpeerinfo outputs so that `transport_protocol_type` isn't stuck at 'detecting'. This is done by adding a wait_until statement till `transport_protocol_type = v2` so that bitcoind waits until the v2 handshake is complete. (on the python side, this is ensured by default since `wait_for_handshake = True` inside `add_p2p_connection()`) ACKs for top commit: Sjors: ACK 0487f91a2046c0d6f91ccaedeb00fbefa635c66d mzumsande: Code Review ACK 0487f91a2046c0d6f91ccaedeb00fbefa635c66d achow101: ACK 0487f91a2046c0d6f91ccaedeb00fbefa635c66d vasild: ACK 0487f91a2046c0d6f91ccaedeb00fbefa635c66d Tree-SHA512: 44dd646a61cd38da243f527df7321e22d1821c2b090be43673027746098caf450c6671708ed731ba257952df6b5886e64c9c2f9686a82f6ef0f25780b7a87d3d
2024-02-29test: Fix intermittent failure in rpc_net.py --v2transportstratospher
Make sure that v2 handshake is complete before comparing getpeerinfo outputs so that `transport_protocol_type` isn't stuck at 'detecting'. - on the python side, this is ensured by default `wait_for_handshake = True` inside `add_p2p_connection()`. - on the c++ side, add a wait_until statement till `transport_protocol_type = v2` so that v2 handshake is complete. Co-Authored-By: Martin Zumsande <mzumsande@gmail.com>
2024-02-28Merge bitcoin/bitcoin#29412: p2p: Don't process mutated blocksAva Chow
d8087adc7ebd4ea05dd3843e5a92e8115fd7bbcc [test] IsBlockMutated unit tests (dergoegge) 1ed2c9829700054526156297552bb47230406098 Add transaction_identifier::size to allow Span conversion (dergoegge) 1ec6bbeb8d27d31647d1433ccb87b362f6d81f90 [validation] Cache merkle root and witness commitment checks (dergoegge) 5bf4f5ba32da4627f152b54d266df9b2aa930457 [test] Add regression test for #27608 (dergoegge) 49257c0304828a185c273fcb99742c54bbef0c8e [net processing] Don't process mutated blocks (dergoegge) 2d8495e0800f5332748cd50eaad801ff77671bba [validation] Merkle root malleation should be caught by IsBlockMutated (dergoegge) 66abce1d98115e41f394bc4f4f52341960ddc839 [validation] Introduce IsBlockMutated (dergoegge) e7669e1343aec4298fd30d539847963e6fa5619c [refactor] Cleanup merkle root checks (dergoegge) 95bddb930aa72edd40fdff52cf447202995b0dce [validation] Isolate merkle root checks (dergoegge) Pull request description: This PR proposes to check for mutated blocks early as a defense-in-depth mitigation against attacks leveraging mutated blocks. We introduce `IsBlockMutated` which catches all known forms of block malleation and use it to do an early mutation check whenever we receive a `block` message. We have observed attacks that abused mutated blocks in the past, which could have been prevented by simply not processing mutated blocks (e.g. https://github.com/bitcoin/bitcoin/pull/27608 for which a regression test is included in this PR). ACKs for top commit: achow101: ACK d8087adc7ebd4ea05dd3843e5a92e8115fd7bbcc maflcko: ACK d8087adc7ebd4ea05dd3843e5a92e8115fd7bbcc 🏄 fjahr: Code review ACK d8087adc7ebd4ea05dd3843e5a92e8115fd7bbcc sr-gi: Code review ACK https://github.com/bitcoin/bitcoin/commit/d8087adc7ebd4ea05dd3843e5a92e8115fd7bbcc Tree-SHA512: 618ff4ea7f168e10f07504d3651290efbb1bb2ab3b838ffff3527c028caf6c52dedad18d04d3dbc627977479710930e200f2dfae18a08f627efe7e64a57e535f
2024-02-28test: make sure keypool sizes do not change on ↵UdjinM6
`getrawchangeaddress`/`getnewaddress` failures
2024-02-27doc: Fix Broken LinksJustin Dhillon
2024-02-27[test] Add regression test for #27608dergoegge
2024-02-27Merge bitcoin/bitcoin#29358: test: use v2 everywhere for P2PConnection if ↵fanquake
--v2transport is enabled bf5662c678455fb47c402f8520215726ddfe7a94 test: enable v2 for python p2p depending on global --v2transport flag (Martin Zumsande) 6e9e39da434f8dafacee4cba068daf499bdb4cc2 test: Don't use v2transport when it's too slow. (Martin Zumsande) 87549c8f89fe8c9f404b74c5a40b5ee54c5a966d test: enable p2p_invalid_messages.py with v2transport (Martin Zumsande) 5fc9db504b9ac784019e7e8c215c31abfccb62b6 test: enable p2p_sendtxrcncl.py with v2transport (Martin Zumsande) Pull request description: #24748 added v2 transport to the python `P2PConnection`, but so far each test that wants to make use of it needs to enable it on an individual basis. This PR changes it so that if the test suite is run with `--v2transport` option, v2 is used in each test by default, not only for connections between two bitcoind instances as before, but also wherever `P2PConnection` is used. Individual tests can override this global option. To do that, a few tests need to be adjusted. In addition, I added a commit to always use v1 in a few select subtests that send a large number of large messages (e.g. large reorgs). These tests don't have a fundamental problem with v2 but become very slow due to the unoptimised python ChaCha20 implementation (~30 minutes on my computer, so probably not suitable to be run in the CI). As a result, `python3 test_runner.py --v2transport` should succeed and use `v2` everywhere (unless v1 is chosen explicitly). [Edit]: To make the "test each commit" CI pass, several test fixes were squashed into the last commit, which actually enables v2 p2p for `P2PConnection`. I have an unsquashed version at https://github.com/mzumsande/bitcoin/tree/202401_bip324_alltests_unsquashed, in case that helps with review. ACKs for top commit: fjahr: tACK bf5662c678455fb47c402f8520215726ddfe7a94 vasild: ACK bf5662c678455fb47c402f8520215726ddfe7a94 stratospher: reACK bf5662c6. theStack: Tested ACK bf5662c678455fb47c402f8520215726ddfe7a94 Tree-SHA512: 4f5a08248ba8a755f7d0f48deb2b79bef03292345cacb7deef01be955481093800e4e56ff218ea56734eef5de1fb3ab0f04657447ea27d393bb536539d7b289d
2024-02-26Merge bitcoin/bitcoin#29467: test: Fix intermittent issue in interface_rest.pyfanquake
faeed91c0be6e5dda4790522d0dc999afd869d11 test: Fix intermittent issue in interface_rest.py (MarcoFalke) Pull request description: Fixes: ``` test 2024-02-22T16:15:37.465000Z TestFramework (ERROR): Assertion failed Traceback (most recent call last): File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 131, in main self.run_test() File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/interface_rest.py", line 340, in run_test assert_equal(json_obj, mempool_info) File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/util.py", line 57, in assert_equal raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args)) AssertionError: not({'loaded': True, 'size': 3, 'bytes': 312, 'usage': 3600, 'total_fee': Decimal('0.00093600'), 'maxmempool': 300000000, 'mempoolminfee': Decimal('0.00001000'), 'minrelaytxfee': Decimal('0.00001000'), 'incrementalrelayfee': Decimal('0.00001000'), 'unbroadcastcount': 1, 'fullrbf': False} == {'loaded': True, 'size': 3, 'bytes': 312, 'usage': 3600, 'total_fee': Decimal('0.00093600'), 'maxmempool': 300000000, 'mempoolminfee': Decimal('0.00001000'), 'minrelaytxfee': Decimal('0.00001000'), 'incrementalrelayfee': Decimal('0.00001000'), 'unbroadcastcount': 0, 'fullrbf': False}) ``` https://cirrus-ci.com/task/4852944378527744?logs=ci#L4436 ACKs for top commit: m3dwards: ACK https://github.com/bitcoin/bitcoin/pull/29467/commits/faeed91c0be6e5dda4790522d0dc999afd869d11 mzumsande: ACK faeed91c0be6e5dda4790522d0dc999afd869d11 Tree-SHA512: 513422229db45d2586c554b9a466e86848bfcf5280b0f000718cbfc44d93dd1af69e19a56f6ac578f5d7aada74ab0c90d4a9e09a324062b6f9ed239e5e34f540
2024-02-23test: Add option to skip unit tests for the test runnerMartin Zumsande
2024-02-22test: Fix intermittent issue in interface_rest.pyMarcoFalke
2024-02-21Merge bitcoin/bitcoin#29400: test: Fix SegwitV0SignatureMsg nLockTime signednessAva Chow
fab15723b0518acbb1015e64df47dcac0187e92f test: Fix SegwitV0SignatureMsg nLockTime signedness (MarcoFalke) Pull request description: It is unsigned in Bitcoin Core, so the tests should match it: https://github.com/bitcoin/bitcoin/blob/5b8990a1f3c49b0b02b7383c69e95320acbda13e/src/script/interpreter.cpp#L1611 The bug was introduced when the code was written in 330b0f31ee5719d94f9e52dfc83c5d82168241f9. (Lowercase `i` means signed, see https://docs.python.org/3/library/struct.html#format-characters) ACKs for top commit: epiccurious: Tested ACK fab15723b0518acbb1015e64df47dcac0187e92f. Eunovo: Tested ACK https://github.com/bitcoin/bitcoin/pull/29400/commits/fab15723b0518acbb1015e64df47dcac0187e92f achow101: ACK fab15723b0518acbb1015e64df47dcac0187e92f Tree-SHA512: 68cb8582f6af22e6abb2fc9d6770277501baa0f9873e2e8d47699e87096fc5d4b9de45fa07199757b6e945c99d4c4ea95f01478322f2c093ef95cf5e0c78522b
2024-02-20test: assert rpc error for addnode v2transport not enabledkevkevin
Added coverage for the addnode rpc when v2transport is not enabled but is set as true when calling addnode rpc
2024-02-15rpc: Fixed signed integer overflow for large feeratesMarcoFalke
2024-02-13Merge bitcoin/bitcoin#29394: test, assumeutxo: Add test to ensure failure ↵fanquake
when mempool not empty 8d20602e555dfe026b421363ee32cfca17c674d8 test, assumeutxo: Add test to ensure failure when mempool not empty (Hernan Marino) Pull request description: Add a test to ensure that loadtxoutset fails when the node's mempool is not empty, as suggested by maflcko here: https://github.com/bitcoin/bitcoin/pull/27596#discussion_r1344713537 ACKs for top commit: maflcko: re-ACK 8d20602e555dfe026b421363ee32cfca17c674d8 BrandonOdiwuor: ACK 8d20602e555dfe026b421363ee32cfca17c674d8 Tree-SHA512: 97c9668c0f38897934bf0d326515d898d4e682ff219deba9d751b35125b5cf33d51c9df116a74117ecf0394f28995a3d0cae1266b1e5acb4365ff4f309ce3f6c
2024-02-13Merge bitcoin/bitcoin#29424: v3 followupsfanquake
6b161cb82a9766ef814f05e5b8f019f15d5ee14d [test] second child of a v3 tx can be replaced individually (glozow) 5c998a696c7a4ff2a91fe3d8c7177d2b806eb7ac [refactor] use MAX_PUBKEYS_PER_MULTISIG instead of magic numbers in test (glozow) a9346421db813ebb1233f6477fe924e2f7c562ad [test] PackageV3Checks with inheritance violation in mempool ancestor (glozow) 63b62e123e38cb92c2135e63eb1a5b760c11dd4e [doc] fix docs and comments from v3 (glozow) Pull request description: Addresses final comments from #28948: - thread at https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1483245289, using https://github.com/ismaelsadeeq/bitcoin/commit/87fc7f0a8d8ad1fc2af60ee6cc678df5e6fb01dd with some modifications - https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1483769698 - https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1483776227 - https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1484427635 - https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1484467280 - https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1484531064 - https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1484992098 - https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1484992336 - https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1484994642 ACKs for top commit: instagibbs: ACK 6b161cb82a9766ef814f05e5b8f019f15d5ee14d sdaftuar: utACK 6b161cb82a9766ef814f05e5b8f019f15d5ee14d Tree-SHA512: 584fce7810f4d704ee6ab51fdc7d42bab342140cae3d076f89b5e1966dd1dd8293cb25b3121e41a4dcd65f9d4a735102b9ab2e90f98aa770b84e21f4d35d63d3
2024-02-13Merge bitcoin/bitcoin#29425: test: fix intermittent failure in ↵fanquake
wallet_reorgrestore.py 44d11532f80705b790bc6e28df9a96ac54b25f9b test: fix intermittent failure in wallet_reorgrestore.py (Martin Zumsande) Pull request description: By adding a missing `sync_blocks` call. There was a race at `node2` between connecting the block produced by `node0`, and using `-generate` to create new blocks itself. In the failed run, block generation started before connecting the block, resulting in a final block height that was smaller by 1 than expected. See https://github.com/bitcoin/bitcoin/issues/29392#issuecomment-1939541603 for a more detailed analysis of the failed run. Can be reproduced by adding a sleep to [this spot](https://github.com/bitcoin/bitcoin/blob/6ff0aa089c01ff3e610ecb47814ed739d685a14c/src/validation.cpp#L4217) in `ChainstateManager::ProcessNewBlock()`: ``` if (util::ThreadGetInternalName() == "msghand") { std::this_thread::sleep_for(0.2s); } ``` which fails for me on master and succeeds with the fix. Fixes #29392 ACKs for top commit: maflcko: lgtm ACK 44d11532f80705b790bc6e28df9a96ac54b25f9b Tree-SHA512: c08699e5ae348d4c0626022b519449d052f511d3f44601bcd8dac836a130a3f67fca149532e1e3690367ebfdcbcdd32e527170d039209c1f599ce861136ae29f
2024-02-12test: fix intermittent failure in wallet_reorgrestore.pyMartin Zumsande
...by adding a missing sync_blocks call. There was a race at node2 between connecting the block produced by node 0, and using -generate to create new blocks itself. In the failed run, the latter happened first, resulting in a final block height that was smaller by 1 than expected.
2024-02-12Merge bitcoin/bitcoin#28987: wallet: simplify and batch zap wallet txes processAva Chow
9a3c5c8697659e34d0476103af942a4615818f4e scripted-diff: rename ZapSelectTx to RemoveTxs (furszy) 83b762845f5804f23b63526d403b2f327fe99632 wallet: batch and simplify ZapSelectTx process (furszy) 595d50a1032ad7ffa9945464c86aa57f16665e93 wallet: migration, remove extra NotifyTransactionChanged call (furszy) a2b071f9920c2f4893afcc43a152f593c03966bf wallet: ZapSelectTx, remove db rewrite code (furszy) Pull request description: Work decoupled from #28574. Brother of #28894. Includes two different, yet interconnected, performance and code improvements to the zap wallet transactions process. 1) As the goal of the `ZapSelectTx` function is to erase tx records that match any of the inputted hashes. There is no need to traverse the whole database record by record. We could just check if the tx exist, and remove it directly by calling `EraseTx()`. 2) Instead of performing single write operations per removed tx record, this PR batches them all within a single atomic db txn. Moreover, these changes will enable us to consolidate all individual write operations that take place during the wallet migration process into a single db txn in the future. ACKs for top commit: achow101: ACK 9a3c5c8697659e34d0476103af942a4615818f4e josibake: ACK https://github.com/bitcoin/bitcoin/pull/28987/commits/9a3c5c8697659e34d0476103af942a4615818f4e Tree-SHA512: fb2ecc48224c400ab3b1fbb32e174b5b13bf03794717727f80f01f55fb183883b067a68c0a127b2de8885564da15425d021a96541953bf38a72becc2e9929ccf
2024-02-12test: enable v2 for python p2p depending on global --v2transport flagMartin Zumsande
This changes the default behavior, individual tests can overwrite this option. As a result, it is possible to run the entire test suite with --v2transport, and all connections to the python p2p will then use it. Also adjust several tests that are already running with --v2transport in the test runner (although they actually made v1 connection before this change). This is done in the same commit so that there isn't an intermediate commit in which the CI fails.
2024-02-12[test] second child of a v3 tx can be replaced individuallyglozow
Co-authored-by: ismaelsadeeq <ask4ismailsadiq@gmail.com>
2024-02-12[refactor] use MAX_PUBKEYS_PER_MULTISIG instead of magic numbers in testglozow
2024-02-12Merge bitcoin/bitcoin#29399: test: Fix utxo set hash serialisation signednessfanquake
fa0ceae970242d8d6bdef150c98f04c67b06e20c test: Fix utxo set hash serialisation signedness (MarcoFalke) Pull request description: It is unsigned in Bitcoin Core, so the tests should match it: https://github.com/bitcoin/bitcoin/blob/5b8990a1f3c49b0b02b7383c69e95320acbda13e/src/kernel/coinstats.cpp#L54 Large positive values for the block height are too difficult to hit in tests, but it still seems fine to fix this. The bug was introduced when the code was written in 6ccc8fc067bf516cda7bc5d7d721945be5ac2003. (Lowercase `i` means signed, see https://docs.python.org/3/library/struct.html#format-characters) ACKs for top commit: epiccurious: Tested ACK fa0ceae970242d8d6bdef150c98f04c67b06e20c. fjahr: utACK fa0ceae970242d8d6bdef150c98f04c67b06e20c Tree-SHA512: ab4405c74fb191fff8520b456d3a800cd084d616bb9ddca27d56b8e5c8969bd537490f6e204c1870dbb09a3e130b03b22a27b6644252a024059c200bbd9004e7
2024-02-09Merge bitcoin/bitcoin#28948: v3 transaction policy for anti-pinningAva Chow
29029df5c700e6940c712028303761d91ae15847 [doc] v3 signaling in mempool-replacements.md (glozow) e643ea795e4b6fea4a6bbb3d72870ee6a4c836b1 [fuzz] v3 transactions and sigop-adjusted vsize (glozow) 1fd16b5c62f54c7f4c60122acd65d852f63d1e8b [functional test] v3 transaction submission (glozow) 27c8786ba918a42c860e6a50eaee9fdf56d7c646 test framework: Add and use option for tx-version in MiniWallet methods (MarcoFalke) 9a1fea55b29fe025355b06b45e3d77d192acc635 [policy/validation] allow v3 transactions with certain restrictions (glozow) eb8d5a2e7d939dd3ee683486e98702079e0dfcc0 [policy] add v3 policy rules (glozow) 9a29d470fbb62bbb27d517efeafe46ff03c25f54 [rpc] return full string for package_msg and package-error (glozow) 158623b8e0726dff7eae4288138f1710e727db9c [refactor] change Workspace::m_conflicts and adjacent funcs/structs to use Txid (glozow) Pull request description: See #27463 for overall package relay tracking. Delving Bitcoin discussion thread: https://delvingbitcoin.org/t/v3-transaction-policy-for-anti-pinning/340 Delving Bitcoin discussion for LN usage: https://delvingbitcoin.org/t/lightning-transactions-with-v3-and-ephemeral-anchors/418 Rationale: - There are various pinning problems with RBF and our general ancestor/descendant limits. These policies help mitigate many pinning attacks and make package RBF feasible (see #28984 which implements package RBF on top of this). I would focus the most here on Rule 3 pinning. [1][2] - Switching to a cluster-based mempool (see #27677 and #28676) requires the removal of CPFP carve out, which applications depend on. V3 + package RBF + ephemeral anchors + 1-parent-1-child package relay provides an intermediate solution. V3 policy is for "Priority Transactions." [3][4] It allows users to opt in to more restrictive topological limits for shared transactions, in exchange for the more robust fee-bumping abilities that offers. Even though we don't have cluster limits, we are able to treat these transactions as having as having a maximum cluster size of 2. Immediate benefits: - You can presign a transaction with 0 fees (not just 1sat/vB!) and add a fee-bump later. - Rule 3 pinning is reduced by a significant amount, since the attacker can only attach a maximum of 1000vB to your shared transaction. This also enables some other cool things (again see #27463 for overall roadmap): - Ephemeral Anchors - Package RBF for these 1-parent-1-child packages. That means e.g. a commitment tx + child can replace another commitment tx using the child's fees. - We can transition to a "single anchor" universe without worrying about package limit pinning. So current users of CPFP carve out would have something else to use. - We can switch to a cluster-based mempool [5] (#27677 #28676), which removes CPFP carve out [6]. [1]: Original mailing list post and discussion about RBF pinning problems https://gist.github.com/glozow/25d9662c52453bd08b4b4b1d3783b9ff, https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2022-January/019817.html [2]: A FAQ is "we need this for cluster mempool, but is this still necessary afterwards?" There are some pinning issues that are fixed here and not fully fixed in cluster mempool, so we will still want this or something similar afterward. [3]: Mailing list post for v3 https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2022-September/020937.html [4]: Original PR #25038 also contains a lot of the discussion [5]: https://delvingbitcoin.org/t/an-overview-of-the-cluster-mempool-proposal/393/7 [6]: https://delvingbitcoin.org/t/an-overview-of-the-cluster-mempool-proposal/393#the-cpfp-carveout-rule-can-no-longer-be-supported-12 ACKs for top commit: sdaftuar: ACK 29029df5c700e6940c712028303761d91ae15847 achow101: ACK 29029df5c700e6940c712028303761d91ae15847 instagibbs: ACK 29029df5c700e6940c712028303761d91ae15847 modulo that Tree-SHA512: 9664b078890cfdca2a146439f8835c9d9ab483f43b30af8c7cd6962f09aa557fb1ce7689d5e130a2ec142235dbc8f21213881baa75241c5881660f9008d68450
2024-02-09wallet: batch and simplify ZapSelectTx processfurszy
The goal of the function is to erase the wallet transactions that match the inputted hashes. There is no need to traverse the database, reading record by record, to then perform single entry removals for each of them. To ensure consistency and improve performance, this change-set removes all tx records within a single atomic db batch operation, as well as it cleans up code, improves error handling and simplifies the transactions removal process entirely. This optimizes the removal of watch-only transactions during the wallet migration process and the 'removeprunedfunds' RPC command.
2024-02-09test, assumeutxo: Add test to ensure failure when mempool not emptyHernan Marino
2024-02-08Merge bitcoin/bitcoin#28996: test: maxuploadtarget: check for mempool msg ↵Ava Chow
disconnect if limit is reached, improve existing test coverage b58f009d9585aab775998644de07e27e2a4a8045 test: check that mempool msgs lead to disconnect if uploadtarget is reached (Sebastian Falbesoner) dd5cf38818d1e3f6cf583e2b242afd0da68b290a test: check for specific disconnect reasons in feature_maxuploadtarget.py (Sebastian Falbesoner) 73d737211536de5b834f21016c5549e52de11374 test: verify `-maxuploadtarget` limit state via `getnettotals` RPC result (Sebastian Falbesoner) Pull request description: This PR improves existing and adds new test coverage for the `-maxuploadtarget` mechanism (feature_maxuploadtarget.py) in the following ways, one commit each: * verify the uploadtarget state via the `getnettotals` RPC (`uploadtarget` result field): https://github.com/bitcoin/bitcoin/blob/160d23677ad799cf9b493eaa923b2ac080c3fb8e/src/rpc/net.cpp#L581-L582 Note that reaching the total limit (`target_reached` == True) always implies that the historical blocks serving limits is also reached (`serve_historical_blocks` == False), i.e. it's impossible that both flags are set to True. * check for peer's specific disconnect reason (in this case, `"historical block serving limit reached, disconnect peer"`): https://github.com/bitcoin/bitcoin/blob/160d23677ad799cf9b493eaa923b2ac080c3fb8e/src/net_processing.cpp#L2272-L2280 * add a test for a peer disconnect if the uploadtarget is reached and a `mempool` message is received (if bloom filters are enabled): https://github.com/bitcoin/bitcoin/blob/160d23677ad799cf9b493eaa923b2ac080c3fb8e/src/net_processing.cpp#L4755-L4763 Note that another reason for disconnect after receiving a MEMPOOL msg of a peer is if bloom filters are disabled on the node. This case is already covered in the functional test `p2p_nobloomfilter_messages.py`. ACKs for top commit: maflcko: lgtm ACK b58f009d9585aab775998644de07e27e2a4a8045 achow101: ACK b58f009d9585aab775998644de07e27e2a4a8045 sr-gi: tACK [b58f009](https://github.com/bitcoin/bitcoin/commit/b58f009d9585aab775998644de07e27e2a4a8045) Tree-SHA512: 7439134129695c9c3a7ddc5e39f2ed700f91e7c91f0b7a9e0a783f275c6aa2f9918529cbfd38bb37f9139184e05e0f0354ef3c3df56da310177ec1d6b48b43d0
2024-02-08Merge bitcoin/bitcoin#29372: test: fix intermittent failure in ↵Ava Chow
`rpc_setban.py --v2transport`, run it in CI cc87ee4c3934028e78a59de509951ff7226ec80d test: fix intermittent failure in rpc_setban.py --v2transport (Martin Zumsande) Pull request description: This test failed for me on master locally: The reason is that when initiating a v2 connection and being immediately disconnected, a node cannot know if the disconnect happens because the peer only supports v1, or because it has banned you, so it schedules to reconnect with v1. If the test doesn't wait for that, the reconnect can happen at a bad time, resulting in failure in a later `connect_nodes` call. Also add the test with `--v2transport` to the test runner because banning with v2 seems like a useful thing to have test coverage for. ACKs for top commit: delta1: tested ACK cc87ee4c3934028e78a59de509951ff7226ec80d epiccurious: Concept ACK cc87ee4c3934028e78a59de509951ff7226ec80d. achow101: ACK cc87ee4c3934028e78a59de509951ff7226ec80d stratospher: tested ACK cc87ee4. nice find! Tree-SHA512: ae234d9b771d9c9c11501ddd93c99cf93257c999de3da62280d4d51806cd246b289c10a5f41fa7d5651b2fb4fdaee753f5b2d6939a99f89d71aa012af4a4d231
2024-02-08[functional test] v3 transaction submissionglozow
Co-authored-by: Suhas Daftuar <sdaftuar@gmail.com> Co-authored-by: Gregory Sanders <gsanders87@gmail.com>
2024-02-08test framework: Add and use option for tx-version in MiniWallet methodsMarcoFalke
2024-02-07Merge bitcoin/bitcoin#29112: sqlite: Disallow writing from multiple ↵Ryan Ofsky
`SQLiteBatch`s cfcb9b1ecf9be5267487713fa1e112ca09a1ae55 test: wallet, coverage for concurrent db transactions (furszy) 548ecd11553bea28bc79e6f9840836283e9c4e99 tests: Test for concurrent writes with db tx (Ava Chow) 395bcd245476d7c83abf2d82e31e90b6d0c432f3 sqlite: Ensure that only one SQLiteBatch is writing to db at a time (Ava Chow) Pull request description: The way that we have configured SQLite to run means that only one database transaction can be open at a time. Typically, each individual read and write operation will be its own transaction that is opened and committed automatically by SQLite. However, sometimes we want these operations to be batched into a multi-statement transaction, so `SQLiteBatch::TxnBegin`, `SQLiteBatch::TxnCommit`, and `SQLiteBatch::TxnAbort` are used to manage the transaction of the database. However, once a db transaction is begun with one `SQLiteBatch`, any operations performed by another `SQLiteBatch` will also occur within the same transaction. Furthermore, those other `SQLiteBatch`s will not be expecting a transaction to be active, and will abort it once the `SQLiteBatch` is destructed. This is problematic as it will prevent some data from being written, and also cause the `SQLiteBatch` that opened the transaction in the first place to be in an unexpected state and throw an error. To avoid this situation, we need to prevent the multiple batches from writing at the same time. To do so, I've implemented added a `CSemaphore` within `SQLiteDatabase` which will be used by any `SQLiteBatch` trying to do a write operation. `wait()` is called by `TxnBegin`, and at the beginning of `WriteKey`, `EraseKey`, and `ErasePrefix`. `post()` is called in `TxnCommit`, `TxnAbort` and at the end of `WriteKey`, `EraseKey`, and `ErasePrefix`. To avoid deadlocking on ` TxnBegin()` followed by a `WriteKey()`, `SQLiteBatch will now also track whether a transaction is in progress so that it knows whether to use the semaphore. This issue is not a problem for BDB wallets since BDB uses WAL and provides transaction objects that must be used if an operation is to occur within a transaction. Specifically, we either pass a transaction pointer, or a nullptr, to all BDB operations, and this allows for concurrent transactions so it doesn't have this problem. Fixes #29110 ACKs for top commit: josibake: ACK https://github.com/bitcoin/bitcoin/pull/29112/commits/cfcb9b1ecf9be5267487713fa1e112ca09a1ae55 furszy: ACK cfcb9b1ecf9be5267487713fa1e112ca09a1ae55 ryanofsky: Code review ACK cfcb9b1ecf9be5267487713fa1e112ca09a1ae55. This looks great and I think it is ready for merge. Just holding off because josibake seemed ready to review https://github.com/bitcoin/bitcoin/pull/29112#issuecomment-1930372190 and might have more feedback. Tree-SHA512: 2dd5a8e76df52451a40e0b8a87c7139d68a0d8e1bf2ebc79168cc313e192dab87cfa4270ff17fea4f7b370060d3bc9b5d294d50f7e07994d9b5a69b40397c927
2024-02-07Merge bitcoin/bitcoin#29363: test: Fix CPartialMerkleTree.nTransactions ↵fanquake
signedness facafa90f7a1eee452f9baf8a1b65a2edac0982b test: Fix CPartialMerkleTree.nTransactions signedness (MarcoFalke) Pull request description: It is unsigned in Bitcoin Core, so the tests should match it: https://github.com/bitcoin/bitcoin/blob/aa9231fafe45513134ec8953a217cda07446fae8/src/merkleblock.h#L59 Large positive values, or "negative" values, are rejected anyway, but it still seems fine to fix this. The bug was introduced when the code was written in d280617bf569f84457eaea546541dc74c67cd1e4. (Lowercase `i` means signed, see https://docs.python.org/3/library/struct.html#format-characters) ACKs for top commit: theStack: LGTM ACK facafa90f7a1eee452f9baf8a1b65a2edac0982b Empact: ACK facafa90f7a1eee452f9baf8a1b65a2edac0982b Tree-SHA512: 35ac11bb5382dffe132bfae6097efc343ef6c06b1b4b1545130ca27b228ca6894679004862fee921b095172abaddbef5972c24d9bc195ce970f35643bd4a0f09
2024-02-07test: Fix SegwitV0SignatureMsg nLockTime signednessMarcoFalke
2024-02-07test: Fix utxo set hash serialisation signednessMarcoFalke
2024-02-06test: Don't use v2transport when it's too slow.Martin Zumsande
Sending multiple large messages is rather slow with the non-optimized python implementation of ChaCha20. Apart from the slowness, these tests would also run successfully with v2.
2024-02-06test: enable p2p_invalid_messages.py with v2transportMartin Zumsande
by disabling some sub-tests that test v1-specific features, and adapting others to v2.
2024-02-06test: enable p2p_sendtxrcncl.py with v2transportMartin Zumsande
By adding to the test framework a wait until the v2 handshake is completed, so that p2p_sendtxrcncl.py (which doesn't need to be changed itself) doesnt't send out any other messages before that.
2024-02-06tests: Test for concurrent writes with db txAva Chow
There are occasions where a multi-statement tx is begun in one batch, and a second batch is created which does a normal write (without a multi-statement tx). These should not conflict with each other and all of the data should end up being written to disk.
2024-02-06Merge bitcoin/bitcoin#29356: test: make v2transport arg in addconnection ↵glozow
mandatory and few cleanups e7fd70f4b6b163f4ad5b25b4da7fa79899245235 [test] make v2transport arg in addconnection mandatory and few cleanups (stratospher) Pull request description: - make `v2transport` argument in `addconnection` regression-testing only RPC mandatory. https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1470738750 - previously it was an optional arg with default `false` value. - only place this RPC is used is in the [functional tests](https://github.com/bitcoin/bitcoin/blob/11b436a66af3ceaebb0f907878715f331516a0bc/test/functional/test_framework/test_node.py#L742) where we always pass the appropriate `v2transport` option to the RPC anyways. (and that too just for python dummy peer(`P2PInterface`) and bitcoind(`TestNode`) interactions) - rename `v2_handshake()` to `_on_data_v2_handshake()` https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1466958424 - more compact return statement in `wait_for_reconnect()` https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1466979708 - assertion to check that empty version packets are received from `TestNode`. ACKs for top commit: glozow: ACK e7fd70f4b6 theStack: Code-review ACK e7fd70f4b6b163f4ad5b25b4da7fa79899245235 mzumsande: Code Review ACK e7fd70f4b6b163f4ad5b25b4da7fa79899245235 Tree-SHA512: e66e29baccd91e1e4398b91f7d45c5fc7c2841d77d8a6178734586017bf2be63496721649da91848dec71da605ee31664352407d5bb896e624cc693767c61a1f
2024-02-06Merge bitcoin/bitcoin#29353: test: p2p: adhere to typical VERSION message ↵glozow
protocol flow c340503b67585b631909584f1acaa327f5af25d3 test: p2p: adhere to typical VERSION message protocol flow (Sebastian Falbesoner) 7ddfc28309e6b70c273112a93f01e518409ceaa0 test: p2p: process post-v2-handshake data immediately (Sebastian Falbesoner) b198b9c2ce28463f7c6a2b4cf08d64138c8509ee test: p2p: introduce helper for sending prepared VERSION message (Sebastian Falbesoner) Pull request description: This PR addresses a quirk in the test framework's p2p implementation regarding the version handshake protocol: Currently, the VERSION message is sent immediately after an inbound connection (i.e. TestNode outbound connection) is made. This doesn't follow the usual protocol flow where the initiator sends a version first, the responder processes that and only then responds with its own version message. Change that accordingly by only sending immediate VERSION message for outbound connections (or after v2 handshake for v2 connections, respectively), and sending out VERSION message as response for incoming VERSION messages (i.e. in the function `on_version`) for inbound connections. I first stumbled upon this issue through reading comment https://mirror.b10c.me/bitcoin-bitcoin/24748/#discussion_r1465420112 (see last paragraph) and recently again in the course of working on a v2-followup for #29279, where this causes issues for TestNode outbound connections that disconnect *before* sending out their own version message. Note that these changes lead to slightly more code in some functional tests that override the `on_version` method, as the version reply has to be sent explicitly now, but I think is less confusing and reflects better what is actually happening. ACKs for top commit: epiccurious: utACK c340503b67585b631909584f1acaa327f5af25d3 stratospher: tested ACK c340503b67585b631909584f1acaa327f5af25d3. very useful to have since we'd want real node behaviour! mzumsande: ACK c340503b67585b631909584f1acaa327f5af25d3 sr-gi: tACK https://github.com/bitcoin/bitcoin/commit/c340503b67585b631909584f1acaa327f5af25d3 Tree-SHA512: 63eac287d3e1c87a01852bfd9f0530363354bbb642280298673b9c8817056356373adf348955c4e92af95c7c6efa8cc515cee2892e9f077bfbe1bce8e97ad082
2024-02-06test: speedup bip324_cipher.py unit testSebastian Falbesoner
Executing the unit tests for the bip324_cipher.py module currently takes quite long (>60 seconds on my notebook). Most time here is spent in empty plaintext/ciphertext encryption/decryption loops: .... for _ in range(msg_idx): enc_aead.encrypt(b"", b"") ... for _ in range(msg_idx): enc_aead.decrypt(b"", bytes(16)) ... Their sole purpose is increasing the FSChaCha20Poly1305 packet counters in order to trigger rekeying, i.e. the actual encryption/decryption is not relevant, as the result is thrown away. This commit speeds up the tests by supporting to pass "None" as plaintext/ciphertext, indicating to the routines that no actual encryption/decryption should be done. master branch: $ python3 -m unittest ./test/functional/test_framework/crypto/bip324_cipher.py .. ---------------------------------------------------------------------- Ran 2 tests in 64.658s PR branch: $ python3 -m unittest ./test/functional/test_framework/crypto/bip324_cipher.py .. ---------------------------------------------------------------------- Ran 2 tests in 0.822s
2024-02-05test: check that mempool msgs lead to disconnect if uploadtarget is reachedSebastian Falbesoner
Note that another reason for disconnect after receiving a MEMPOOL msg of a peer is if bloom filters are disabled on the node. This case is covered in the functional test `p2p_nobloomfilter_messages.py`.
2024-02-05test: check for specific disconnect reasons in feature_maxuploadtarget.pySebastian Falbesoner
This ensures that the disconnect happens for the expected reason and also makes it easier to navigate between implementation and test code, i.e. both the questions "do we have test coverage for this disconnect?" (from an implementation reader's perspective) and "where is the code handling this disconnect?" (from a test reader's perspective) can be answered simply by grep-ping the corresponding debug message.
2024-02-05test: verify `-maxuploadtarget` limit state via `getnettotals` RPC resultSebastian Falbesoner
2024-02-05Merge bitcoin/bitcoin#29354: test: Assumeutxo with more than just coinbase ↵glozow
transactions fa5cd66f0a47d1b759c93d01524ee4558432c0cc test: Assumeutxo with more than just coinbase transactions (MarcoFalke) Pull request description: Currently the AU tests only check that loading a txout set with only coinbase outputs works. Fix that by adding other transactions. ACKs for top commit: jamesob: ACK https://github.com/bitcoin/bitcoin/pull/29354/commits/fa5cd66f0a47d1b759c93d01524ee4558432c0cc glozow: concept ACK fa5cd66f0a47d1b759c93d01524ee4558432c0cc Tree-SHA512: e090c41f73490ad72e36c478405bfd0716d46fbf5a131415095999da6503094a86689a179a84addae3562b760df64cdb67488f81692178c8ca8bf771b1e931ff
2024-02-02Merge bitcoin/bitcoin#28868: wallet: Fix migration of wallets with txs that ↵Ryan Ofsky
have both spendable and watchonly outputs 4da76ca24725eb9ba8122317e04a6e1ee14ac846 test: Test migration of tx with both spendable and watchonly (Ava Chow) c62a8d03a862fb124b4f4b88efd61978e46605f8 wallet: Keep txs that belong to both watchonly and migrated wallets (Ava Chow) 71cb28ea8cb579ac04cefc47a57557c94080d1af test: Make sure that migration test does not rescan on reloading (Ava Chow) 78ba0e6748d2a519a96c41dea851e7c43b82f251 wallet: Reload the wallet if migration exited early (Ava Chow) 9332c7edda79a39bb729b71b6f8db6a9d37343bb wallet: Write bestblock to watchonly and solvable wallets (Ava Chow) Pull request description: A transaction does not necessarily have to belong to either the migrated wallet (with the private keys) and the watchonly wallet (with watchonly things), it could have multiple outputs with each isminetype. So we should be putting such transactions in one or the other wallet, but rather putting it in both. I've added a test for this behavior, however the test also revealed a few other issues. Notably, it revealed that `migratewallet` would have the watchonly wallet rescan from genesis when it is reloaded at the end of migration. This could be a cause for migration appearing to be very slow. This is resolved by first writing best block records to the watchonly and solvable wallets, as well as updating the test to make sure that rescans don't happen. The change to avoid rescans also found an issue where some of our early exits would result in unloading the wallet even though nothing happened. So there is also a commit to reload the wallet for such early exits. ACKs for top commit: ryanofsky: Code review ACK 4da76ca24725eb9ba8122317e04a6e1ee14ac846. This looks great. The code is actually cleaner than before, two bugs are fixed, and the test checking for rescanning is pretty clever and broadens test coverage. furszy: Code review ACK 4da76ca2 Tree-SHA512: 5fc210cff16ca6720d7b2d0616d7e3f295c974147854abc704cf99a3bfaad17572ada084859e7a1b1ca94da647ad130303219678f429b7995f85e040236db35c
2024-02-02test: fix intermittent failure in rpc_setban.py --v2transportMartin Zumsande
When initiating a v2 connection and being immediately disconnected, a node cannot know if the disconnect happens because the peer only supports v1, or because it has banned you, so it schedules to reconnect with v1. If the test doesn't wait for that, the reconnect can happen at a bad time, resulting in failure in a later connect_nodes call. Also add the test with --v2transport to the test runner.
2024-02-01tests: Test that descriptors flag is set for migrated blank walletsAva Chow
2024-02-01test: Test migration of tx with both spendable and watchonlyAva Chow
generated by cgit v1.2.3 (git 2.26.2) at 2024-11-10 01:11:06 +0000