Removed the wallet restrictions for rpc_deprecated.py and added specific test case for the current deprecated rpc.
skip_test_if_missing_module will skip the whole test when the wallet is missing, even if a part of the test is non-wallet related.
Rename the `_randomize_credentials` parameter to Proxy's constructor to
`tor_stream_isolation` to make it more clear, and more specific what its
purpose is.
Also change all call sites to use a named parameter.
7bb83f6718 test: create assert_not_equal util and add to where imports are needed (kevkevin)
Pull request description:
In the functional tests there are lots of cases where we assert != which we now swap with assert_not_equal to be more readable
This is motivated/uses logic from this PR which was closed https://github.com/bitcoin/bitcoin/pull/28528
This partially helps https://github.com/bitcoin/bitcoin/issues/23119
I've broken it up to just `assert_not_equal` to keep the PR smaller as suggested in https://github.com/bitcoin/bitcoin/pull/28528#issuecomment-1959945805
I can create follow up PR's if this is wanted
ACKs for top commit:
hodlinator:
re-ACK 7bb83f6718
ryanofsky:
Code review ACK 7bb83f6718. Only change since last review is fixing error message formatting and passing it as a keyword argument
janb84:
Re-ACK [7bb83f6](7bb83f6718)
Tree-SHA512: de09f41a690033a5b61e6f861d3bd69a32b889d6655a28fbc0d5cfac9f7ec9c642432967d33913970882b4cfdd47bdd377d0ddc44e25976cbaa49f7f9d8f7b10
8e4a0ddd50 torcontrol: Add comment explaining Proxy credential randomization for Tor privacy (Eval EXEC)
ec5c0b26ce torcontrol: Define tor reply code as const to improve maintainability (Eval EXEC)
Pull request description:
This PR want to:
1. replace tor repy code with const to improve out maintainability.
2. cherry-picked https://github.com/bitcoin/bitcoin/pull/31973 , add comment to explain Proxy credential randomization for Tor privacy
ACKs for top commit:
hodlinator:
re-ACK 8e4a0ddd50
laanwj:
re-ACK 8e4a0ddd50
Tree-SHA512: 038daa6508ca88fceed5c8e155430614cb56976f36d1f8baee5114bca1141122cf94f51814a869848b3442691ee765cbf609cf946b2b35d5135015a9b749d917
6afffba34e contrib: (asmap) add docs about encode and decode commands (jurraca)
67d5cc2a06 contrib: (asmap) add documentation on diff and diff-addrs commands (jurraca)
e047b1deca contrib: (asmap) add diff-addrs example to README (jurraca)
Pull request description:
This README was a little sparse in my opinion, and was missing a mention of the `diff-addrs` command.
The README updates add background and examples for each command, split in two sections (encode/decode and diff/diff-addrs). This is intended to help people know how and when to run the commands available in the `asmap-tool.py` script.
However, I could use some confirmation on the behavior of the `--fill` flag. It's true that files generated with this flag set cannot be used to diff files after the fact, but i don't quite follow what the fill flag does to make that true. sipa could you maybe provide some insight?
ACKs for top commit:
fjahr:
re-ACK 6afffba34e
brunoerg:
reACK 6afffba34e
laanwj:
re-ACK 6afffba34e
Tree-SHA512: 073e8d7255f7270aa2f5a070332872f5fa6fbe6532eee1f7e3e4158ac0125a49c155f4933bf00655ff3a89f666f3f3bea521e70c516ab09a448845016d2b880a
0ff66b1c4a fuzz: coinselection: cover `SetBumpFeeDiscount` (brunoerg)
Pull request description:
`SetBumpFeeDiscount` sets the bump fee discount which is used to calculate the waste. We currently have no fuzz coverage for this function, so this PR adds it by calling `SetBumpFeeDiscount` before `RecalculateWaste`.
ACKs for top commit:
marcofleon:
ACK 0ff66b1c4a
Tree-SHA512: d5c1d97daaeb7f9b096bf9bdf6374b8a674a75f464e2b9bb3e1e1774a5805b22840ca1f31bae63f106640d9ce27a99432c3034524340be91c235f6ec3b185cff
8284229a28 refactor: deduplicate anchor witness program bytes (`0x4e,0x73`) (Sebastian Falbesoner)
41f2f058d0 test: add missing segwitv1 test cases to `script_standard_tests` (Sebastian Falbesoner)
Pull request description:
Currently we have two segwitv1 output script types that are considered standard:
- `TxoutType::WITNESS_V1_TAPROOT` (P2TR): witness program has size 32 (introduced with taproot soft-fork)
- `TxoutType::ANCHOR` (P2A): witness program is {0x4e, 0x7e} (introduced with #30352)
This PR adds them to the script standardness unit tests where missing, i.e. for using them with the `ExtractDestination` and `GetScriptForDestination` functions.
ACKs for top commit:
rkrux:
ACK 8284229a28
instagibbs:
reACK 8284229a28
hodlinator:
Code Review ACK 8284229a28
Tree-SHA512: d4a3b47fd31ba33f62d4367811e72a7f442c01b046b0a7217a66be0b9dea5c9041eebfe812c31839ec0f0b14c56948c7c016d3d2de79283583ad8e32c192c6ff
aa7a898c23 doc: use testnet4 in developer docs (Sjors Provoost)
6c217d22fd test: use testnet4 in argsman test (Sjors Provoost)
7c200ece80 test: use testnet4 in key_io_valid.json (Sjors Provoost)
d424bd5941 test: drop unused testnet3 magic bytes (Sjors Provoost)
8cfc09fafe test: cover testnet4 magic in assumeutxo.py (Sjors Provoost)
4281e3603a zmq: use testnet4 in zmq_sub.py example (Sjors Provoost)
Pull request description:
In preparation for dropping testnet3 entirely in #31974 this PR migrates a few things to testnet4:
* the ZMQ examples
* developer docs
* various unit tests
* the snapshot magic byte check in `feature_assumeutxo.py`
It drops `testnet3` from `MAGIC_BYTES` in the test framework, since no test uses it.
ACKs for top commit:
fjahr:
re-ACK aa7a898c23
maflcko:
lgtm ACK aa7a898c23🔊
hodlinator:
re-ACK aa7a898c23
Tree-SHA512: 235f74273234e8fb2aedf0017dea5c16bb9813ec7a1f89a51abe85691f09830a5ead834115d7db0936e12e55a40bc81888856a8002fe507c1474407e77f8b9fb
In the functional tests there are lots of cases where we assert != which
this new util will replace, we also are adding the imports and the new assertion
Threads may execute their function any time after they are spawned, so
coverage could be non-deterministic.
Fix this,
* for the script check worker threads by disabling them while fuzzing.
* for the scheduler thread by waiting for it to fully start and run the
service queue.
This makes it humanly possible to track progress as only "[N/M]"-lines are printed as long as we succeed.
Also, use char (a, b) to indicate run_id instead of u8 (0, 1).
Also, use emojis to indicate final success or error.
Co-Authored-By: Hodlinator <172445034+hodlinator@users.noreply.github.com>
25b56fd9b4 ci: Test cross-built Windows executables on Windows natively (Hennadii Stepanov)
3501bca8c7 ci: Move "Windows cross" job from Cirrus CI to GHA CI (Hennadii Stepanov)
f8619196ce ci: Use `bash` by default for all platforms (Hennadii Stepanov)
Pull request description:
This PR enables on the CI tests of cross-compiled Windows binaries on Windows.
It is important to have such tests in CI because the release binaries for Windows are also cross-compiled.
Two functional tests, `wallet_migration.py` and `wallet_multiwallet.py`, are temporarily disabled. They require fixes, such as https://github.com/bitcoin/bitcoin/pull/31410, and adjustments for error message handling. Re-enabling these tests will be addressed in follow-up PRs.
Resolves https://github.com/bitcoin/bitcoin/issues/31071.
ACKs for top commit:
davidgumberg:
tested reACK 25b56fd9b4
hodlinator:
re-ACK 25b56fd9b4
willcl-ark:
utACK 25b56fd9b4
maflcko:
review-only ACK 25b56fd9b4🍎
Tree-SHA512: fb9150807b7ebb248e8f4fe7b16e5179251e7be9336459287787f27e542583d73d937e6969667fd836378b676bb9be7f66756dc1abca8a01364bc9ee3e3720a5
This has no functional affect, as the any CBlockIndex*s which
to_mark_failed is set to will already have been marked failed.
Also prevents a situation where block already marked as
BLOCK_FAILED_CHILD is again unconditionally marked as
BLOCK_FAILED_VALID in the final |= BLOCK_FAILED_VALID.
invalid_block ----------> block_index
- before this commit, only if block_index is not invalid, it will mark
block_index as BLOCK_FAILED_CHILD
- it's possible that block_index encountered is invalid and was marked
as BLOCK_FAILED_VALID previously
- in this case, correctly update BlockStatus of block_index by
clearing BLOCK_FAILED_VALID and then setting it to BLOCK_FAILED_CHILD
when a block is invalidated using InvalidateBlock, check that:
1. it's status is BLOCK_FAILED_VALID
2. it's children's status is BLOCK_FAILED_CHILD
and not BLOCK_FAILED_VALID
3. it's ancestors are valid
this block of code is not reached on master since other than
initialisation, all other iterations have invalid_walk_tip
and to_mark_failed pointers in some form of this layout
where 1, 2, 3 and 4 are block heights.
invalid_walk_tip
↓
1 <- 2 <- 3 <- 4
↑
to_mark_failed
fix it so that blocks are correctly marked as BLOCK_FAILED_CHILD
if it's a descendant of BLOCK_FAILED_VALID block.
This change stresses that all ZMQ messages share the same structure
and that they differ only in the format of the bodies. Previously this
was not clear.
Further it removes the notion of endianness of 32-byte hashes,
as it was misleading, and replaces it with the term 'reversed byte
order' (as opposed to natural or normal byte order produced by hashing
functions).
Additionally, it states that ZMQ 32-byte hashes are in the same format
as in RPC. Previously it incorrectly stated that the two were in
different formats.
* Range-for avoids ++i/i++ debate and decreases linecount.
* seen_multipath is only used if multipath_segment_index hasn't already been set. Rename it to seen_substitutes to better describe what it does, now that the context implies its involved in multipath.
57d8b1f1b3 cmake: Avoid fuzzer "multiple definition of `main'" errors (Ryan Ofsky)
Pull request description:
This change builds libraries with `-fsanitize=fuzzer-no-link` instead of `-fsanitize=fuzzer` when the cmake `-DSANITIZERS=fuzzer` option is specified. This is necessary to make fuzzing and IPC cmake options compatible with each other and avoid CI failures in #30975 which enables IPC in the fuzzer CI build:
https://cirrus-ci.com/task/5366255504326656?logs=ci#L2817https://cirrus-ci.com/task/5233064575500288?logs=ci#L2384
The failures can also be reproduced by checking out #31741 and building with `cmake -B build -DBUILD_FOR_FUZZING=ON -DSANITIZERS=fuzzer -DENABLE_IPC=ON` with this fix reverted.
The fix updates the cmake build so when `-DSANITIZERS=fuzzer` is specified, the fuzz test binary is built with `-fsanitize=fuzzer` (so it can use libFuzzer's main function), and libraries are built with `-fsanitize=fuzzer-no-link` (so they can be linked into other executables with their own main functions).
Previously when `-DSANITIZERS=fuzzer` was specified, `-fsanitize=fuzzer` was applied to ALL libraries and executables. This was inappropriate because it made it impossible to build any executables other than the fuzz test executable without triggering link errors:
- `` multiple definition of `main' ``
- `` "undefined reference to `LLVMFuzzerTestOneInput' ``
if they depended on any libraries instrumented for fuzzing.
This was especially a problem when the `ENABLE_IPC` option was set because it made building the `mpgen` code generator impossible so nothing else that depended on generated sources, including the fuzz test binary, could be built either.
This commit was previously part of https://github.com/bitcoin/bitcoin/pull/31741 and had some discussion there starting in https://github.com/bitcoin/bitcoin/pull/31741#pullrequestreview-2619682385
---
This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/issues/28722).
ACKs for top commit:
hebasto:
ACK 57d8b1f1b3, tested on Ubuntu 24.04.
Tree-SHA512: 4011adbc0b08742e83cf7c0560d3d5b5694a863358e6ac9a21239626b4a8fedceca66db34b5a46136a7b26849bb1d8710c894689322ae97e1c407687c3f57d50
ae6b6ea296 wallet: remove redundant `Assert` call when block is disconnected (rkrux)
Pull request description:
It was highlighted in a PR discussion previously that the recently moved `Assert` macro call inside the block disconnected loop had been redundant for quite a while because of the presence of the `assert` macro call at the start of the function. Therefore, it is removed now.
refs #https://github.com/bitcoin/bitcoin/pull/31757#discussion_r1995416821
ACKs for top commit:
fjahr:
utACK ae6b6ea296
l0rinc:
crACK ae6b6ea296
hodlinator:
Code Review ACK ae6b6ea296
Prabhat1308:
Code Review ACK [`ae6b6ea`](ae6b6ea296)
Tree-SHA512: 6bbced88f4b39afcacefb7babe97c180a397d9cd55f18c4c2875bd594547dcdccb2059ac32495e0e8d4e7263b4c1349ca80b2f0fbd46b4450d1d847ba5abd903
d065208f0f test: get rid of redundant TODO tag (Chandra Pratap)
Pull request description:
The `FEE` parameter in `test/functional/feature_dbcrash.py::generate_small_transaction()` is not a fee rate, but an absolute fee. Hence, it doesn't make sense to replace it with node relay based fee calculation. Get rid of the TODO comment suggesting otherwise.
ACKs for top commit:
maflcko:
lgtm ACK d065208f0f
Tree-SHA512: f2b7f51ffb23de8e14ca071edd731410176a20750115a65db0ae67714389e03ffe1593ce88368e96d211329bd93c772f665de7c3a59b932681bc5b80db908d9f
This abstracts out the finding of the connected component that includes
a given element from FindConnectedComponent (which just finds any connected
component).
Use this in the txgraph fuzz test, which was effectively reimplementing this
logic. At the same time, improve its performance by replacing a vector with a
set.
ca55613fd1 test: Add functional test for bitcoin-chainstate (TheCharlatan)
3f9c716e7f test: Fix docstring for cmake migration (TheCharlatan)
Pull request description:
While the `bitcoin-chainstate` utility is not shipped in a release, it is the only current utility directly using the bitcoin kernel library. Adding a simple test for it is useful for checking that the library is actually usable. The test is also useful in future to demonstrate that the `bitcoin-chainstate` binary using the API for the kernel library introduced in #30595 actually works and offers similar features.
ACKs for top commit:
laanwj:
Code review ACK ca55613fd1
maflcko:
ACK ca55613fd1🎭
kevkevinpal:
ACK ca55613fd1
Tree-SHA512: 282627f5fac868a84aab9962ef2cbd3a8d3941d9f9dc2a3f26db1e7706ffa8051637ab5f8372676800e426e077ca40449a9e3e42a003048472339d81ed81bca8
a82829f37e test: simplify (w)txid checks by avoiding .calc_sha256 calls (Sebastian Falbesoner)
346a099fc1 test: avoid unneeded hash -> uint256 -> hash roundtrips (Sebastian Falbesoner)
Pull request description:
In the functional test framework we currently have a strong tendency to treat and store identifiers that result from hash functions (e.g. (w)txids, block hashes) as integers, which seems an unnatural and confusing choice. Hashes are just pseudo-random sequences of bytes, and there is usually no need to apply integer operations on them; the only exceptions I could think of is PoW-verification of block hashes with the less-than (`<`) operator, or interpreting the byte-string as scalar in the EC-context for e.g. key derivation.
I'd hence argue that most uses of `ser_uint256`/`uint256_from_str` and txid conversions via `int(txid/blockhash, 16)` are potential code smells and should be reduced to a minimum long-term if possible. This PR is a first step into this direction, intentionally kept small with (what I think) uncontroversial changes for demonstration purposes, to check out if other contributors are interested in this. A next step could be to change the classes of primitives (CTransaction, CBlock etc.) and network messages (msg_) to store hash results as actual bytes (maybe in a class wrapping the bytes that offers conversion from/to human-readable strings [1], for easier interaction with RPC calls and debug outputs) rather than ints. But that would of course need larger, potentially more controversial changes, and its questionable if its really worth the effort.
[1] unfortunately, txids and block hashes are shown to user in reverse byte order, so e.g. a txid_bytes->txid_str conversion is not just a simple `txid_bytes.hex()`, but a `txid_bytes[::-1].hex()`
ACKs for top commit:
maflcko:
review ACK a82829f37e🐘
rkrux:
Concept and utACK a82829f37e
ryanofsky:
Code review ACK a82829f37e. Nice changes, and sorry about the false bug report
Tree-SHA512: bb0465802d743a495207800f922b65f49ed0d20552f95bb0bee764944664092aad74812e29df6e01ef40bcb8f9bc6c84c7e9cbbe6f008ee1a14d94ed88e698b4
It was highlighted in a PR discussion previously that the recently
moved `Assert` macro call inside the block disconnected loop had
been redundant for quite a while because of the presence of the
`assert` macro call at the start of the function. Therefore, it
is removed now.
refs #https://github.com/bitcoin/bitcoin/pull/31757#discussion_r1995416821
