bf0d257c11 net: un-default the OpenNetworkConnection()'s proxy_override argument (Eugene Siegel)
5a3756d150 test: add a regression test for private broadcast v1 retries (Vasil Dimov)
ab35a028ed test: make reusable filling of a node's addrman (Vasil Dimov)
2333be9cbc test: make reusable starting a standalone P2P listener (Vasil Dimov)
2ffa81fac4 test: make reusable SOCKS5 server starting (Vasil Dimov)
32d072a49f doc: add release notes for #35319 (Vasil Dimov)
d01b461f71 net: ensure no direct private broadcast connections (Vasil Dimov)
fd230f942d net: use the proxy if overriden when doing v2->v1 reconnections (Vasil Dimov)
Pull request description:
This PR includes https://github.com/bitcoin/bitcoin/pull/35319 and on top of that adds a regression functional test.
The functional test exercises the relevant code paths without modifying non-test code. To do that it does:
* Add a bunch of IPv4 addresses to the node's addrman (they will be added without P2P_V2 flag).
* Get them to report P2P_V2 in their service flags and connect to each one, so that the flags
in addrman are updated to contain P2P_V2.
* Get one successful connection to a Tor peer (.onion) so that bitcoind assumes the configured
Tor proxy works and is indeed a proxy to the Tor network. This will make it open private
broadcast connections also to IPv4 addresses via that proxy.
* Start some private broadcast connections.
* Remember the destination IPv4 address of the first connection and get it to fail the v2
transport.
* Wait for a subsequent connection also through the Tor proxy to the same IPv4 and expect
it to be v1, i.e. the v2->v1 downgrade retry.
The test fails without the fix - the v1 retry never arrives to the Tor proxy. And passes with the fix. The fix is in the first commit here and in https://github.com/bitcoin/bitcoin/pull/35319, can remove it by `git show fd230f942d | git apply -R`.
ACKs for top commit:
Crypt-iQ:
reACK bf0d257c11
andrewtoth:
ACK bf0d257c11
instagibbs:
ACK bf0d257c11
sedited:
utACK bf0d257c11
Tree-SHA512: 11e89be36577199e0312e5e63efeac04e295faaba1cf1c13a30e683d35f473c8dbb419d1897b0333c2e993c10637adecafcf90fe08c812065c793cbc903744c9
7735c13488 test: run bitcoin-cli -ipcconnect check under valgrind with -datadir (Michael Dietz)
Pull request description:
This case invokes bitcoin-cli via raw subprocess.run() without -datadir, so it reads the default datadir's bitcoin.conf (e.g. ~/.bitcoin) and fails whenever that real config is unusable. Pass the node's -datadir so the check reads the test's own bitcoin.conf and depends only on the build's IPC support, not the host environment.
ACKs for top commit:
maflcko:
lgtm ACK 7735c13488
sedited:
ACK 7735c13488
Tree-SHA512: 5264433e2cd1747b9b9b4437b80f1849b5fa01620dd958c34c124925270dfca85c3809186285bf1dd70a8358552e1ebb589ec84093e11bb0d66cfe10f04dde81
4a6d1458b4 ci: add pyzmq to msan job (fanquake)
c21b58e263 ci: use pyzmq over zmq (fanquake)
Pull request description:
`zmq` seems to be an alias for `pyzmq`, and the project page, https://pypi.org/project/zmq/, states "You are probably looking for pyzmq.". So switch to `pyzmq`, which is what we document, and use in all other jobs.
Also add `pyzmq` to native MSAN, so that `interface_zmq.py` is run.
ACKs for top commit:
sedited:
ACK 4a6d1458b4
Tree-SHA512: 86b96f6b2dca032bf7460113173472c8384834b1f38d961dff1ee325cce75410cc02062da798484f12fb28f676611df7ea406ee8385f9324b298c036826881f9
82901981bf ci: use Warp cache for Docker layers (will)
Pull request description:
We switched to GH cache recently, but the performance is beyond terrible. We have already switched the main cache actions over to warp, and this completes the transfer with the docker buildkit cache to the Warp endpoint.
ACKs for top commit:
m3dwards:
ACK 82901981bf
Tree-SHA512: caee053598c5082b1f0cc52a6abfe6314ac2d5d840238480f1706f55a892f08ca533ac3d5d4f9f20a84958df1c9100ae00518686072515a1011be994619b4df9
7249b376a0 opt: Skip UTXOs with worse waste, same eff_value (Murch)
5204291860 opt: Skip evaluation of equivalent input sets (Murch)
ba1807b981 coinselection: Track effective_value lookahead (Murch)
fa226ab902 coinselection: BnB skip exploring high waste (Murch)
7ecea1dc5d coinselection: Track whether BnB completed (Murch)
3ca0f36164 coinselection: rewrite BnB in CoinGrinder-style (Murch)
2e73739837 coinselection: Track BnB iteration count in result (Murch)
Pull request description:
This PR rewrites the implementation of the BnB coinselection algorithm
to skip the duplicate evaluation of previously visited input selections.
In the original implementation of BnB, the state of the search is
backtracked by explicitly walking back to the omission branch and then
testing again. This retests an equivalent candidate set as before, e.g.,
after backtracking from {ABC}, it would evaluate {AB_}, before trying
{AB_D}, but {AB_} is equivalent to {AB} which was tested before.
CoinGrinder tracks the state of the search instead by remembering which
UTXO was last added and explicitly shifting from that UTXO directly to
the next, so after {ABC}, it will immediately move on to {AB_D}. We
replicate this approach here.
As fewer nodes are visited, this approach will enumerate more possible
combinations than the original implementation given the same limit for
iterations.
ACKs for top commit:
achow101:
ACK 7249b376a0
w0xlt:
reACK 7249b376a0
Tree-SHA512: fd5851ceea3a3a4699fc062254fa5438daa4275b4d52325983e63670040cf0ba35112be9e63813d8f30b38993c031f3df343b2152eb8c068d272fbff72d1881a
801e3bfe38 chainparams: add overloads for RegTest and SigNet with no options (Antoine Poinsot)
4995c00a9c chainparams: make deployment configuration available on all test networks (Antoine Poinsot)
df7ed5f355 chainparams: encapsulate deployment configuration logic (Antoine Poinsot)
Pull request description:
It's sometimes useful to test a deployment on other networks than regtest. This may be e.g. because regtest lacks a property relevant for the test, or simply because the test aims to be portable while regtest is Bitcoin Core specific.
This PR makes it possible to set the `-vbparams` and `-testactivationheight` options on any network in unit tests, and on any **test** network as a startup option.
This is preparatory work for a BIP 54 implementation, but may be useful separately.
ACKs for top commit:
edilmedeiros:
utACK 801e3bfe38
achow101:
ACK 801e3bfe38
sedited:
ACK 801e3bfe38
instagibbs:
ACK 801e3bfe38
Tree-SHA512: 10649dc9bbc70a830bb0c4b1c965de5bf2e6be1a6c2832bdf11e9248dacb4a1f60421b410d0284213e88de6c54218252ae5de423b4c6e659d10bab1cbe7e7e87
107d4178d9 versionbits: update VersionBitsCache doc comment to match current behaviour (Antoine Poinsot)
94e3ac0b21 doc: release notes and bips doc update for #34779 (Antoine Poinsot)
1d5240574a qa: test we don't warn for ignored unknown version bits deployments (Antoine Poinsot)
f802edf57c versionbits: Limit live activation params and activation warnings per BIP323 (Anthony Towns)
Pull request description:
This implements https://github.com/bitcoin/bips/pull/2116, which repurposes 24 version bits as extra nonce space for miners rather than soft fork deployment coordination. 24 bits allows a miner to perform up to 72 PH before needing a fresh job from its controller. The current 16 bits in use by miners only allow up to 280 TH, which [apparently led some ASIC designers to start rolling the timestamp field](https://github.com/bitaxeorg/ESP-Miner/pull/1553#issuecomment-3937736319) on their beefier machines.
Mailing list discussion available [here](https://gnusha.org/pi/bitcoindev/6fa0cb45-37d6-4b41-9ff8-03730fd96d6e@mattcorallo.com/). A previous shot at this is https://github.com/bitcoin/bitcoin/pull/13972 (with a smaller extranonce space).
This change only affects the warning logic.
ACKs for top commit:
ajtowns:
ACK 107d4178d9
achow101:
ACK 107d4178d9
sedited:
Re-ACK 107d4178d9
optout21:
ACK 107d4178d9
Tree-SHA512: cfaf5d7de1e8c020a4d7f4b1096b6c3e0e3b41ea840a4652ebcdabc345c5c557161c8304f1d7d6de541a2bf1df3c855ad7b64e49dd8c8af3937876d134bb5aba
Speeds of 1MB/s and 15 minute cached docker image pulls during builds
are not uncommon.
Warp runners provide a local GitHub Actions cache protocol proxy for
Docker layer cache traffic. Point BuildKit's gha cache backend at that
proxy on Warp runners so cached image layers do not have to be fetched
from GitHub's slower cache service.
Add a default for provider so other users (e.g. qa-assets) don't have to
update this unless they use custome runners.
55d37546fa Bugfix: GUI/Intro: Handle errors from SelectParams the same as if during InitConfig (Luke Dashjr)
Pull request description:
Without this, invalid vbparams just silently exit with no message
ACKs for top commit:
sedited:
ACK 55d37546fa
hebasto:
ACK 55d37546fa, tested on Fedora 44.
Tree-SHA512: 0508ca64c86a651b9b21ae2a1e26dfb84c0dbb0b20d309da499545be2733d42c98012b84d81177ba0635d0d8bce87c888b256a014e2065bd4a80db88e73ec3d4
zmq seems to be an alias for pyzmq, and the project page,
https://pypi.org/project/zmq/, states "You are probably looking for
pyzmq.". So switch to pyzmq, which is what we document, and use in all
other jobs.
8cb8653a22 fuzz: target concurrent leveldb reads (Andrew Toth)
6609088fe6 fuzz: extract ConsumeDBParams helper (Andrew Toth)
Pull request description:
Inspired by https://github.com/bitcoin/bitcoin/pull/31132#issuecomment-4054461591.
We currently do concurrent leveldb reads when accessing our indexes.
1. `txindex` - we call `FindTx()` from multiple RPC threads.
2. `blockfilterindex` - we call `LookupFilter/Header()` concurrently from `msghand` thread for p2p requests as well as RPC threads.
3. `coinstatsindex` - we call `LookUpStats()` from multiple RPC threads.
4. `txospenderindex` - we call `FindSpender()` from multiple RPC threads.
We also read from our chainstate and blocks index while background compactions are writing.
While OSS-Fuzz does cover leveldb (https://github.com/google/oss-fuzz/blob/master/projects/leveldb/fuzz_db.cc), it doesn't cover multi threaded access. Without a deterministic hypervisor this fuzz harness won't be deterministic, but we can at least run it with TSan to get a higher confidence that the synchronization code in leveldb is correct. Hopefully other reviewers find this useful.
This harness creates a threadpool with 16 threads, and then creates an in-memory levelDB which it seeds with deterministically random values. It chooses a random set of keys to query. It first performs all queries on the db on a single thread to get a baseline, then synchronizes all threads on a latch so they hit the db at the same time. Each thread performs the same queries, and afterwards are all checked against the baseline.
It uses a `DeterministicEnv` to capture background compaction work when seeding the db, which is also run immediately after the latch is released. This causes a race between compaction and reading, ensuring we exercise many thread synchronization code paths in leveldb.
I ran both TSan and ASan/UBSan overnight with no issues.
ACKs for top commit:
fjahr:
Code review ACK 8cb8653a22
l0rinc:
ACK 8cb8653a22
Tree-SHA512: 2ca31a824715b92e258c84ecf0c762f43ee2a528e3a3192f94d8aaeddf6e99f820a0297ce9efcc95bc32c7ec74489f240a25bab856d724d768117a7d95a33974
2ef6679c2c test: Check that MuSig2 signing does not reuse nonces (Ava Chow)
bb05986c0a musig: Include pubnonce in session id (Ava Chow)
Pull request description:
It is safe to have multiple musig signing sessions over the same message so long as the nonces used are different. Including the pubnonce in the session id allows for multiple simultaneous signing sessions over the same message, rather than asserting when the user tries to do this.
The second commit tests this behavior, both ensuring that there is no crash, and verifying that both sessions produce unique nonces and signatures to verify that no reuse is occurring.
Lastly, the assertion in `SetMuSig2SecNonce` is retained as hitting it now would indicate that a nonce has been reused. We prefer to assert and crash rather than do something that is highly likely to leak a private key.
Fixes#35250
ACKs for top commit:
rkrux:
lgtm ACK 2ef6679c2c
junbyjun1238:
utACK 2ef6679c2c
theStack:
ACK 2ef6679c2c
Tree-SHA512: 9fb60b68ebe0ea9656408afb65b9ec9f280632e1bb84a4821b074c8d8569847845f7c29da800c757b9ddf3aa31aa890dd9e3646cf119917a714e7daf20be2198
This case invoked bitcoin-cli via raw subprocess.run() without the
valgrind wrapper (bypassing valgrind) and without -datadir (so it read
the default datadir's bitcoin.conf, e.g. ~/.bitcoin, and failed whenever
that real config was unusable). Build the command like the rest of the
framework: prepend binaries.valgrind_cmd and pass the node's -datadir so
the check runs under valgrind and depends only on the build's IPC
support, not the host environment.
Pull the inline DBParams construction out of TestDbWrapper into a shared
ConsumeDBParams() helper. This is a pure refactor with no behavior change,
preparing for an additional harness that needs to build the same params.
6c525c2ec1 wallet: unfriend LegacyDataSPKM and DescriptorScriptPubKeyMan classes (rkrux)
Pull request description:
After #28333, `LegacyDataSPKM` doesn't need to use the private or protected members
of `DescriptorScriptPubKeyMan` class such as `AddDescriptorKeyWithDB` and
`TopUpWithDB`. Moreover, these two SPKMs are siblings that inherit from the common
`ScriptPubKeyMan` that have non intersecting use cases semantically. It seems reasonable
to me that they are unfriended so that private members of one are not exposed to
another unnecessarily.
ACKs for top commit:
polespinasa:
ACK 6c525c2ec1
sedited:
ACK 6c525c2ec1
Tree-SHA512: ebcd9b4e202b152b53c390f44cac46748b7fdebd4f854e84d322468a1f117e729d75210db5688dc7f38124280378f62fa469d2e5ee9adb4cb77acb2c73cc8480
c8b8c275fa test: Improve loopback address check in `rcp_bind.py` (xyzconstant)
Pull request description:
A [loopback address](https://www.geeksforgeeks.org/computer-networks/what-is-a-loopback-address/) can range from `127.0.0.0` to `127.255.255.255`. This commit relaxes the loopback check in `rpc_bind.py` by checking whether an IP address (from `all_interfaces()`) starts with `'127.'` instead of strictly matching `'127.0.0.1'`.
Programs like VPNs might add an extra loopback address (e.g., 127.1.130.83), which failed under the previous state. These addresses will now pass with this update.
---
**For context:** I found this while running tests with the Mullvad daemon active. Mullvad adds a custom lo0 interface like `inet 127.141.11.239 netmask 0xff000000` that failed with `--nonloopback`, which should not be the case since the address is a valid loopback IP.
ACKs for top commit:
maflcko:
lgtm ACK c8b8c275fa
willcl-ark:
ACK c8b8c275fa
Tree-SHA512: 3b82002d6bc90cfc4023dd0274a40970abb2dc6a9ced77dd97e275b31340bb657d5222bb55a768ebf71047ac1521dd4ba77fb427398f7cc9857738bcd16c5818
2ce4ae7d8f ci: Add dynamic cache switching to warp cache (will)
Pull request description:
The GHA cache is very slow, taking on the order of minutes to save and
restore from.
Use WarpBuild's cache instead as this is in the same region and should
be much faster.
WarpBuild cache action does not auto-fallback to GHA if not being run on
Warp. To allow fork runs to fallback to GHA caching, whilst minimising
duplication in the action files, create new "interal" actions which
perform the switching logic, and use these in the (renamed) cache|save
actions.
Without this we would need the `if` logic in our prvious actions, 4
times in each of save and restore.
ACKs for top commit:
m3dwards:
ACK 2ce4ae7d8f
Tree-SHA512: 966ed85558e8ebeed20a6fdf065f0838c9bee95f7d0a2fd1c8a46de20b956f5eedb6bf5098d742da20ea80c56069b86ff46f3151936dc28d5e4afa70acc5d988
9c1fcaca5c wallet, test: fix sendall anti-fee-sniping when locktime is not specified (rkrux)
8877eec726 wallet: allow anti-fee-sniping in sendall RPC while not relying on RBF default (rkrux)
Pull request description:
Partially fixes https://github.com/bitcoin/bitcoin/issues/32661.
Prerequisite to #35405.
The test change in the second commit would fail without the presence
of the first commit.
ACKs for top commit:
maflcko:
review ACK 9c1fcaca5c🍅
xyzconstant:
Tested ACK 9c1fcaca5c
polespinasa:
code review ACK 9c1fcaca5c
sedited:
ACK 9c1fcaca5c
Tree-SHA512: 048c1a6c64c104f8c27053e4dea139fb8740f30096f4d85daf23aa53a9ad21f0120151a7d1d6ea19b12d174976999335b3f9ac863800629774d8030b7f34918f
A loopback address can range from `127.0.0.0` to `127.255.255.255`.
This commit relaxes the loopback check in `rpc_bind.py` by checking whether
an IP address (from `all_interfaces()`) starts with `'127.'` instead of
strictly matching `'127.0.0.1'`.
Programs like VPNs might add an extra loopback address (e.g., 127.1.130.83),
which failed under the previous state. These addresses will now pass with this update.
131fa570b9 test: Add test for BuildSkip() and skip heights (optout)
Pull request description:
The skip height values computed by the (internal) function `GetSkipHeight()`, and the `CBlockIndex::BuildSkip()` method are not tested directly, and the skip logic is not well documented. To improve test coverage, a new test is added, to verify `CBlockIndex::BuildSkip()` and the skip height bit-manipulation logic.
Note: the original version contained a test for the complexity of the `GetAncestor()` algorithm, whose performance is greatly determined by the skip height logic. This test was out-scoped. (see: https://github.com/bitcoin/bitcoin/pull/33661#issuecomment-4486353485)
The motivation is to document the skip value computation through a test. (The issued was noticed while reviewing #33515.)
ACKs for top commit:
l0rinc:
ACK 131fa570b9
sipa:
ACK 131fa570b9
Tree-SHA512: 468070946419d9d2891e43ed014b040348fc9d3b35dc21522487ac28e06b6d5d556ac02ebc4fa7761e202fc80f9e9ab7a7ec47c6e05ae55e33950ff5f8f3596f
570a627640 kernel: assert invalid buffer preconditions in `btck_*_create` functions (stringintech)
Pull request description:
The kernel API appears to use `nullptr` returns to report failures that callers may reasonably want to recover from: malformed serialized input, object construction failures, chainstate/load failures, and similar runtime conditions.
The raw create-function buffer checks seem to be a different case. A failure of `ptr == nullptr && len > 0` does not indicate malformed input data or a failure encountered while deserializing or constructing the requested object. Returning `nullptr` for these checks widens the recoverable error surface with cases that are better treated as programmer errors, similar to other asserted preconditions in this API such as invalid indices and impossible enum/flag states.
This change switches those buffer argument checks from `nullptr` returns to assertions in `btck_transaction_create`, `btck_script_pubkey_create`, `btck_block_create`, `btck_block_header_create`, and `btck_chainstate_manager_options_create`. `btck_block_header_create` additionally asserts the pre-existing documented length contract (must be 80 bytes). These functions still return `nullptr` when the provided bytes cannot be parsed or when object creation fails during processing.
I ended up looking at this while working on the `kernel-bindings-tests` spec/schema for `btck_script_pubkey_create`, where treating this path as a regular error did not seem like the right contract: https://github.com/stringintech/kernel-bindings-tests/pull/14#discussion_r3240859568.
ACKs for top commit:
stickies-v:
ACK 570a627640
janb84:
ACK 570a627640
w0xlt:
ACK 570a627640
sedited:
ACK 570a627640
Tree-SHA512: 064d834abe0c27245a144e5290bbeeb510daf9e4d50bb3a8e50bd8a0bf897b3dcf6ad5acfcabf1d8110da120e5e014ee3aea0241c0f181a21c6f3c14dc452ade
The GHA cache is very slow, taking on the order of minutes to save and
restore from.
Use WarpBuild's cache instead as this is in the same region and much
faster.
WarpBuild cache action does not auto-fallback to GHA if not being run on
Warp. To allow fork runs to fallback to GHA caching, whilst minimising
duplication in the action files, create new "interal" actions which
perform the switching logic, and use these in the (renamed) cache|save
actions.
Without this we would need the `if` logic in our prvious actions, 4
times in each of save and restore.
Plumb the provider through into the action, as a composite action can't
read `env` (`GITHUB_OUTPUT`) from previous steps.
53388773af guix: Remove redundant ShellCheck `source` directives (Hennadii Stepanov)
62cf7bc53f guix, refactor: Add `BASE` argument to `*_for_host` functions (Hennadii Stepanov)
5d46429e32 guix, refactor: Move `distsrc_for_host()` to `prelude.bash` (Hennadii Stepanov)
cab65ea9c6 guix, refactor: Move duplicated `profiledir_for_host()` to `prelude.bash` (Hennadii Stepanov)
faa9d4345f guix, refactor: Move duplicated `outdir_for_host()` to `prelude.bash` (Hennadii Stepanov)
6b59fd6b8c guix, refactor: Remove `contains()` function (Hennadii Stepanov)
d4c69a7224 guix, refactor: Remove unused `out_name()` function (Hennadii Stepanov)
Pull request description:
While working on https://github.com/bitcoin/bitcoin/pull/35098, I reviewed my notes regarding a few minor Guix script flaws and decided to address them here.
This PR:
1. Removes unused code.
2. Reduces code duplication.
3. Improves consistency across function usage.
4. Minimizes ShellCheck directive usage.
ACKs for top commit:
fanquake:
ACK 53388773af - the move-only deduplication, and dead code removal seem fine. The other (refactoring) changes seem less well motived
Tree-SHA512: d565e31ba49300f3001b04d3143721aced305f41bca6d04c33dc479d568efb2c06d91758619378199f174a40f1306c9418ae80d5478013a65ad96341bea122b1
fa787043f5 doc: Compress doc/build-unix.md dependency package names into table (MarcoFalke)
Pull request description:
Currently, `doc/build-unix.md` is tediously verbose, because for several Linux distros, it has exact duplicate sections with only the package names adjusted.
This is hard to maintain, and review. Also, it is hard to read, and hard to use, because a one-line copy-paste does not work to fetch the list of packages.
Fix all issues by compressing the 150+ lines into a small table and a short description.
ACKs for top commit:
achow101:
ACK fa787043f5
darosior:
ACK fa787043f5
sedited:
ACK fa787043f5
hebasto:
re-ACK fa787043f5.
Tree-SHA512: a6d7f18392ab5a0d468387ffe4335f71ae9656a100ede3de4118e3ef28814e5a70202ffa55eb0218a07effcde220b680499ea9068fd54b91cac42fca8699febf
fad585b6e5 test: Wait for node exit after crash in verify_utxo_hash (MarcoFalke)
faf1475514 ci: Exclude feature_dbcrash.py under --v2transport --usecli (MarcoFalke)
fac27d702f test: Fix feature_dbcrash.py --usecli intermittent error (MarcoFalke)
fa09de8b68 test: [refactor] Simplify submit_block_catch_error (MarcoFalke)
Pull request description:
This fixes a small intermittent issue that snuck in via commit fa8d4d5c35.
Generally, it seems tedious and brittle trying to enumerate all possible exception types on all platforms and all test configs, all possible errno values, etc.
So just treat any `Exception` as crash, and confirm it in the test. The test would still fail, if a crash did not happen after an Exception, but the failure may be minimally more tedious to debug. I think this is fine, because failures should be rare and having simpler and more flexible test code is preferable.
----
Also, disable the test for now in CI, because it is quite slow.
ACKs for top commit:
willcl-ark:
ACK fad585b6e5
Tree-SHA512: 6ff69a53908b22904780f29def473f8e26c5b608d89420ac76768d06ea3e3394b5d3f4d488100f9d47f943ff5778a555302ccdaebc11fda7c009205b6a6ca05c
ac09260982 test: restore JSONRPCException error format (rkrux)
Pull request description:
This is a follow-up to PR #34575.
ACKs for top commit:
maflcko:
lgtm ACK ac09260982
Tree-SHA512: 15979f4e2c07993f283640ebfe570e9f8d3842a23a8118042f5b618273e0da8a01bcabe1ec90b6cc49ebf28e9819d1b4f077ac18f62f681a4d4f58ad8e11bdb1
This particular test case only needs to ensure that locktime is not
specified in the RPC request, it doesn't need to rely on the wallet optin
RBF default that causes the test to pass coincidentally.
Co-authored-by: maflcko <6399679+maflcko@users.noreply.github.com>
Switch buffer `ptr == nullptr && len > 0` checks from `nullptr` returns to assertions. These checks represent invalid caller preconditions, not failures encountered while deserializing or constructing the requested object. `btck_block_header_create` additionally asserts the pre-existing documented length contract (must be 80 bytes).
This is a follow-up to PR 34575.
Copy is done so that checking of error["code"] in test_node.py
while handling this exception doesn't fail.
Co-authored-by: maflcko <6399679+maflcko@users.noreply.github.com>
Extract the part of `p2p_private_broadcast.py` that starts
listening on a `P2PConnection` object (or its children classes)
and put it into `test_framework/p2p.py`.
Extract the part of `p2p_private_broadcast.py` that configures and
starts the SOCKS5 server into a reusable function and put it into
`test_framework/socks5.py`.
Use bind port 0 to let the OS pick an available port instead of
hackishly assuming that `p2p_port(N)` is available where N is more
than the number of the nodes the test uses.
After PR 28333, `LegacyDataSPKM` doesn't need to use the private or
protected members of `DescriptorScriptPubKeyMan` class such as
`AddDescriptorKeyWithDB` and `TopUpWithDB`. Moreover, these two SPKMs
are siblings that inherit from the common `ScriptPubKeyMan`.
It seems reasonable to me that they are unfriended so that private
members of one are not exposed to another unnecessarily.
In case locktime (and replaceable) not being specified in this RPC request,
the wallet sets the transaction replaceable due to the default value of opt-in
RBF set in the wallet.
This allowed the anti-fee-sniping flow to be executed but in case of
replaceable being set false in the request, anti-fee-sniping flow would be
missed.
This patch fixes it.
fedeff7f20 crypto: disable ASan instrumentation of SSE4 SHA256 for GCC (deadmanoz)
Pull request description:
Fix the runtime crash described in #34881.
Upstream already disables ASan instrumentation for `sha256_sse4::Transform()` under Clang. This extends the same workaround to GCC by adding an `#elif` branch for `__GNUC__` / `__SANITIZE_ADDRESS__` that applies the same `no_sanitize("address")` attribute.
Testing:
- reproduced the crash before the fix with GCC 13, 14, and 15 on Haswell-class machines / guests without SHA-NI (including by forcing the SSE4 implementation on GitHub CI)
- SEGV in debug builds regardless of optimization level (tested `-O0`, `-O1`, `-O2`, `-O3`)
- verified that the GCC + ASan debug configurations that previously crashed pass with this change
Issue #34881 has more details about the issue.
Note: the original Clang code placed the `__attribute__` between the function declarator and the opening brace. GCC's [Attribute Syntax](https://gcc.gnu.org/onlinedocs/gcc/Attribute-Syntax.html) documentation notes that this position in a function definition "may, in future, be permitted," so it is not currently supported. Placing the attribute at the start of the function definition is valid form for both GCC and Clang.
ACKs for top commit:
maflcko:
lgtm ACK fedeff7f20🏒
sedited:
tACK fedeff7f20
Tree-SHA512: d8adda0df140b6c93d18f5ecd096b12012332bb640e678075e668122e596baddcb2182cbaeafe7908ada90b4b5cc776a59dcdfb488229ab6640058ccbbd7ea93
f701cd159a doc: fix typo in release notes of #34917 (rkrux)
7bc39e3d08 wallet, test: add wallet_deprecated_rbf.py for walletrbf deprecated keys & options (rkrux)
2cbbcb5659 wallet, test: remove -deprecatedrpc=bip125 from wallet_send.py (rkrux)
307134bd7e wallet, test: remove -deprecatedrpc=bip125 from wallet_migration.py (rkrux)
3ec550d168 wallet, test: remove -deprecatedrpc=bip125 from wallet_basic.py (rkrux)
a52ea9bff9 wallet, test: remove -walletrbf startup option from wallet_backwards_compatibility.py (rkrux)
42330922dd wallet, test: remove -walletrbf startup option from wallet_backwards_compatibility.py (rkrux)
8cb6e405d8 wallet, test: remove -walletrbf startup option from wallet_listtransactions.py (rkrux)
0ee94b2fef wallet, test: remove -deprecatedrpc=bip125 from wallet_listtransactions.py (rkrux)
5e833e068d wallet, test: -walletrbf startup option from wallet_bumpfee.py (rkrux)
a2a2b1745f wallet, test: remove -walletrbf startup option from rpc_psbt.py (rkrux)
a3fe455a95 wallet: refactor to read -walletrbf only once instead of twice (rkrux)
Pull request description:
Prerequisite to #35404 and #35405.
All these changes address the points raised in the review of PR #34917
here: https://github.com/bitcoin/bitcoin/pull/34917#pullrequestreview-4362148900.
Essentially updating the existing wallet functional tests without using
the -deprecatedrpc=bip125 and -walletrbf startup options. Instead,
these two are added and tested via a singular new
wallet_deprecated_rbf.py test that can be removed easily later when
these startup options are completely removed from the wallet post
deprecation.
ACKs for top commit:
maflcko:
review ACK f701cd159a🌄
achow101:
ACK f701cd159a
Tree-SHA512: 700785062b5de8ee3b6c4f50570b769d56c6c4960f2b6e2a2e71be8085c6b51eaeb34fb158fae76f812fe82791aaa0c0277f964f0472cb0784b86caabe6d4ec9
