On default legacy wallets, the backup filename starts with an "-" due
to the wallet name being empty. This is inconvenient for systems who
treat what follows the initial "-" character as flags.
Github-Pull: #29586
Rebased-From: a951dba3a9
A common issue that our fuzzers keep finding is that outpoints don't
exist in the non witness utxos. Instead of trying to track this down and
checking in various individual places, do the check early during
deserialization.
Github-Pull: #29855
Rebased-From: 9e13ccc50e
53ffd5a410 docs: Fix broken reference to CI setup in test/lint/README.md (naiyoma)
Pull request description:
The current [reference](https://github.com/bitcoin/bitcoin/blob/master/test/ci/lint/04_install.sh
) for CI setup in /test/lint#readme returns a 404.
ACKs for top commit:
fanquake:
ACK 53ffd5a410
Tree-SHA512: 813c19a145f09e7da11963598b70dc438acba784eb722e509d6af59dc3af8f5da97628c454bed2b03cc919689603e070796de2db8d784d9162ae34e7b85a77d9
e67ab174c9 test: fix flaky wallet_send functional test (Max Edwards)
3c49e69670 test: fix weight estimates in functional tests (Max Edwards)
Pull request description:
Fixes: https://github.com/bitcoin/bitcoin/issues/25164
The wallet_send functional test has been flaky due to a slightly overestimated weight calculation. This PR makes the weight calculation more accurate, although occasionally, due to how ECDSA signatures can be different lengths it might slightly over estimate. The assertion in the test can handle this slight variation and so should continue passing.
Update:
Because the signature can be shorter that is used in the weight estimation or the final transaction the estimate could be both slightly smaller or slightly larger.
ACKs for top commit:
achow101:
ACK e67ab174c9
S3RK:
Code review ACK e67ab174c9
Tree-SHA512: 3bf73b355309dce860fa1520afb8461e94268e4bcf0e92a8273c279b41b058c44472cf59daafa15a515529b50bd665b5d498bbe4d934f2315dbe810a05bc73f9
Rather than asserting that the exact fees are the same, check the fee rate rounded to nearest interger. This will account for small differences in fees caused by variability in ECDSA signature lengths.
Updates the weight estimate to be more accurate by removing byte buffers and calculating the length of the count of scriptWitnesses rather than just using the count itself.
a8c3454ba1 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`:
9eeee7caa3/test/functional/test_framework/crypto/bip324_cipher.py (L193-L194)9eeee7caa3/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 a8c3454ba1.
achow101:
ACK a8c3454ba1
marcofleon:
ACK a8c3454ba1. 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
e073f1dfda test: make sure keypool sizes do not change on `getrawchangeaddress`/`getnewaddress` failures (UdjinM6)
367bb7a80c 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 e073f1dfda 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 e073f1dfda
josibake:
ACK e073f1dfda
Tree-SHA512: 1128288a60dd4d8f306ef6f7ac66cdfeae3c9cc35c66ecada2d78fa61ac759f2a757b70fc3976ba8b5081200942b58dfabc184c01ccf911af40ba8c145344651
0487f91a20 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 0487f91a20
mzumsande:
Code Review ACK 0487f91a20
achow101:
ACK 0487f91a20
vasild:
ACK 0487f91a20
Tree-SHA512: 44dd646a61cd38da243f527df7321e22d1821c2b090be43673027746098caf450c6671708ed731ba257952df6b5886e64c9c2f9686a82f6ef0f25780b7a87d3d
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>
d8087adc7e [test] IsBlockMutated unit tests (dergoegge)
1ed2c98297 Add transaction_identifier::size to allow Span conversion (dergoegge)
1ec6bbeb8d [validation] Cache merkle root and witness commitment checks (dergoegge)
5bf4f5ba32 [test] Add regression test for #27608 (dergoegge)
49257c0304 [net processing] Don't process mutated blocks (dergoegge)
2d8495e080 [validation] Merkle root malleation should be caught by IsBlockMutated (dergoegge)
66abce1d98 [validation] Introduce IsBlockMutated (dergoegge)
e7669e1343 [refactor] Cleanup merkle root checks (dergoegge)
95bddb930a [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 d8087adc7e
maflcko:
ACK d8087adc7e🏄
fjahr:
Code review ACK d8087adc7e
sr-gi:
Code review ACK d8087adc7e
Tree-SHA512: 618ff4ea7f168e10f07504d3651290efbb1bb2ab3b838ffff3527c028caf6c52dedad18d04d3dbc627977479710930e200f2dfae18a08f627efe7e64a57e535f
