f700609e8a doc: Release notes for mining IPC interface bump (Ryan Ofsky)
9453c15361 ipc mining: break compatibility with existing clients (version bump) (Sjors Provoost)
70de5cc2d2 ipc mining: pass missing context to BlockTemplate methods (incompatible schema change) (Sjors Provoost)
2278f017af ipc mining: remove deprecated methods (incompatible schema change) (Ryan Ofsky)
c6638fa7c5 ipc mining: provide default option values (incompatible schema change) (Ryan Ofsky)
a4603ac774 ipc mining: declare constants for default field values (Ryan Ofsky)
ff995b50cf ipc test: add workaround to block_reserved_weight exception test (Ryan Ofsky)
b970cdf20f test framework: expand expected_stderr, expected_ret_code options (Ryan Ofsky)
df53a3e5ec rpc refactor: stop using deprecated getCoinbaseCommitment method (Ryan Ofsky)
Pull request description:
This PR increments the field number of the `Init.makeMining` method and makes the old `makeMining` method return an error, so IPC mining clients not using the latest schema file will get an error and not be able to access the Mining interface.
Normally, there shouldn't be a need to break compatibility this way, but the mining interface has evolved a lot since it was first introduced, with old clients using the original methods less stable and performant than newer clients. So now is a good time to introduce a cutoff, drop deprecated methods, and stop supporting old clients which can't function as well.
Bumping the field number is also an opportunity to make other improvements that would be awkward to implement compatibly:
- Making Cap'n Proto default parameter and field values match default values of corresponding C++ methods and structs.
- Adding missing Context parameters to Mining.createNewBlock and checkBlock methods so these methods will be executed on separate execution threads and not block the Cap'n Proto event loop thread.
More details about these changes are in the commit messages.
ACKs for top commit:
Sjors:
ACK f700609e8a
enirox001:
ACK f700609e8a
ismaelsadeeq:
ACK f700609e8a
sedited:
ACK f700609e8a
Tree-SHA512: 0901886af00214c138643b33cec21647de5671dfff2021afe06d78dfd970664a844cde9a1e28f685bb27edccaf6e0c3f2d1e6bb4164bde6b84f42955946e366d
0b96b9c600 Minimize mempool lock, sync txo spender index only when and if needed (sstone)
3d82ec5bdd Add a "tx output spender" index (sstone)
Pull request description:
This PR adds a new "tx output spender" index, which allows users to query which tx spent a given outpoint with the `gettxspendingprevout` RPC call that was added by https://github.com/bitcoin/bitcoin/pull/24408.
Such an index would be extremely useful for Lightning, and probably for most layer-2 protocols that rely on chains of unpublished transactions.
UPDATE: this PR is ready for review and issues have been addressed:
- using a watch-only wallet instead would not work if there is a significant number of outpoints to watch (see https://github.com/bitcoin/bitcoin/pull/24539#issuecomment-1276595646)
- this PR does not require `-txindex` anymore
We use a composite key with 2 parts (suggested by romanz): hash(spent outpoint) and tx position, with an empty value. Average composite key size is 15 bytes.
The spending tx can optionally be returned by `gettxspendingprevout` (even it `-txindex is not set`).
ACKs for top commit:
hodlinator:
re-ACK 0b96b9c600
sedited:
Re-ACK 0b96b9c600
fjahr:
ACK 0b96b9c600
w0xlt:
reACK 0b96b9c600
Tree-SHA512: 95c2c313ef4086e7d5bf1cf1a3c7b91cfe2bb1a0dcb4c9d3aa8a6e5bfde66aaca48d85a1f1251a780523c3e4356ec8a97fe6f5c7145bc6ccb6f820b26716ae01
Adds an outpoint -> txid index, which can be used to find which transactions spent a given output.
We use a composite key with 2 parts (suggested by @romanz): hash(spent outpoint) and tx position, with an empty value.
To find the spending tx for a given outpoint, we do a prefix search (prefix being the hash of the provided outpoint), and for all keys that match this prefix
we load the tx at the position specified in the key and return it, along with the block hash, if does spend the provided outpoint.
To handle reorgs we just erase the keys computed from the removed block.
This index is extremely useful for Lightning and more generally for layer-2 protocols that rely on chains of unpublished transactions.
If enabled, this index will be used by `gettxspendingprevout` when it does not find a spending transaction in the mempool.
24699fec84 doc: Add initial asmap data documentation (Fabian Jahr)
bab085d282 ci: Use without embedded asmap build option in one ci job (Fabian Jahr)
e53934422a doc: Expand documentation on asmap feature and tooling (Fabian Jahr)
6244212a55 init, net: Implement usage of binary-embedded asmap data (Fabian Jahr)
6202b50fb9 build: Generate ip_asn.dat.h during build process (Fabian Jahr)
634cd60dc8 build: Add embedded asmap data (Fabian Jahr)
Pull request description:
This is the final in a series of PRs that implement the necessary changes for embedding of asmap data into the binary. This last part add the initial asmap data, implements the build changes and adds further documentation.
Currently an asmap file needs to be acquired by there user from some location or the user needs to generate one themselves. Then they need to move the file to the right place in datadir or pass the path to the file as `-asmap=PATH` in order to use the asmap feature. The change here allows for builds to embed asmap data into the bitcoind binary which makes it possible to use the feature without handling of the asmap file by the user. If the user starts bitcoind with `-asmap` the embedded data will be used for bucketing of nodes.
The data lives in the repository at `src/node/data/ip_asn.dat` and can be replaced with a new version at any time. The idea is that the data should be updated with every release. By default the data at that location is embedded into the binary but there is also a build option to prevent this (`-DWITH_EMBEDDED_ASMAP=OFF`). In this case the original behavior of the `-asmap` option is maintained.
ACKs for top commit:
achow101:
ACK 24699fec84
sipa:
ACK 24699fec84
hodlinator:
ACK 24699fec84
Tree-SHA512: c2e33dbeea387efdfd3d415432bf8fa64de80f272c1207015ea53b85bb77f5c29f1dae5644513a23c844a98fb0a4bb257bf765f38b15bfc4c41984f0315b4c6a
fb3e1bf9c9 test: check LoadBlockIndex correctly recomputes invalidity flags (stratospher)
29740c06ac validation: remove BLOCK_FAILED_MASK (stratospher)
b5b2956bda validation: reset BLOCK_FAILED_CHILD to BLOCK_FAILED_VALID when loading from disk (stratospher)
37bc207852 validation: stop using BLOCK_FAILED_CHILD (stratospher)
120c631e16 refactor: use clearer variables in InvalidateBlock() (stratospher)
18f11695c7 validation: don't update BLOCK_FAILED_VALID to BLOCK_FAILED_CHILD in InvalidateBlock (stratospher)
Pull request description:
Fixes https://github.com/bitcoin/bitcoin/issues/32173
even though we have a distinction between `BLOCK_FAILED_VALID` and `BLOCK_FAILED_CHILD` in the codebase,
we don't use it for anything. Whenever we check for BlockStatus, we use `BLOCK_FAILED_MASK` which encompasses both of them.
Since there is no functional difference between `BLOCK_FAILED_VALID` and `BLOCK_FAILED_CHILD` and it's added
code complexity to correctly categorise them (ex: https://github.com/bitcoin/bitcoin/pull/31405#discussion_r1914366243, https://github.com/bitcoin/bitcoin/pull/16856#issuecomment-565506585), we could just remove it.
Looking for conceptual feedback on whether it's better to improve handling of `BLOCK_FAILED_CHILD` in the codebase or remove `BLOCK_FAILED_CHILD`.
Of less relevance, but it would also fix a `reconsiderblock` crash that could happen in the situation mentioned in https://github.com/bitcoin/bitcoin/issues/32173#issuecomment-2767030982
Similar attempt in the past in https://github.com/bitcoin/bitcoin/pull/16856#issuecomment-568073859
ACKs for top commit:
stickies-v:
re-ACK fb3e1bf9c9
alexanderwiederin:
ACK fb3e1bf9c9
mzumsande:
re-ACK fb3e1bf9c9
Tree-SHA512: e97b739885c40a8c021966438e9767cc02bc183056236d6a8c64f6819347ae70c0fbcd71cc2528917560d9f4fd56aed45faf1b6c75d98de7b08b621693a97fbc
- there maybe existing block indexes stored in disk with
BLOCK_FAILED_CHILD
- since they don't exist anymore, clean up block index entries with
BLOCK_FAILED_CHILD and reset it to BLOCK_FAILED_VALID.
even though we have a distinction between BLOCK_FAILED_VALID
and BLOCK_FAILED_CHILD in the codebase, we don't use it for
anything. since there's no functional difference between them
and it's unnecessary code complexity to categorise them correctly,
just mark as BLOCK_FAILED_VALID instead.
This change removes deprecated methods from the ipc mining interface.
Warning: This is an intermediate, review-only commit. Binaries built from it
should not be distributed or used to connect to other clients or servers. It
makes incompatible changes to the `mining.capnp` schema without updating the
`Init.makeMining` version, causing binaries to advertise support for a schema
they do not actually implement. Mixed versions may therefore exchange garbage
requests/responses instead of producing clear errors. The final commit in this
series bumps the mining interface number to ensure mismatches are detected.
git-bisect-skip: yes
This commit only declares constants without using them. They will be applied in
seperate commit since changing struct default field values in cap'n proto is
not backwards compatible.
b623fab1ba mining: enforce minimum reserved weight for IPC (Sjors Provoost)
d3e49528d4 mining: fix -blockreservedweight shadows IPC option (Sjors Provoost)
418b7995dd test: have mining template helpers return None (Sjors Provoost)
Pull request description:
Also enforce `MINIMUM_BLOCK_RESERVED_WEIGHT` for IPC clients.
The `-blockreservedweight` startup option should only affect RPC code, because IPC clients (currently) do not have a way to signal their intent to use the node default (the `BlockCreateOptions` struct defaults merely document a recommendation for client software).
Before this PR however, if the user set `-blockreservedweight` then `ApplyArgsManOptions` would cause the `block_reserved_weight` option passed by IPC clients to be ignored. _Users who don't set this value were not affected._
Fix this by making BlockCreateOptions::block_reserved_weight an std::optional.
Internal interface users, such as the RPC call sites, don't set a value so -blockreservedweight is used. Whereas IPC clients do set a value which is no longer ignored.
Test coverage is added, with a preliminary commit that refactors the `create_block_template` and `wait_next_template` helpers.
`mining_basic.py` already ensured `-blockreservedweight` is enforced by mining RPC methods. The second commit adds coverage for Mining interface IPC clients. It also verifies that `-blockreservedweight` has no effect on them.
The third commit enforces `MINIMUM_BLOCK_RESERVED_WEIGHT` for IPC clients. Previously lower values were quietly clamped.
---
Merge order preference: #34452 should ideally go first.
ACKs for top commit:
sedited:
Re-ACK b623fab1ba
ryanofsky:
Code review ACK b623fab1ba. Was rebased and test split up and comment updated since last review.
ismaelsadeeq:
ACK b623fab1ba
Tree-SHA512: 9e651a520d8e4aeadb330da86788744b6ecad8060fa21d50dc8e6012a60083e7b262aaa08a64676b9ef18ba65b651bc1272d8383d184030342e4c0f2c6a9866d
37cc2a2d95 logging: use util/log.h where possible (stickies-v)
bb8e9e7c4c logging: Move message formatting to util/log.h (stickies-v)
001f0a428e move-only: Move logging macros to util/log.h (stickies-v)
94c0adf4e8 move-onlyish: Move logging levels to util/log.h (stickies-v)
56d113cab0 move-only: move logging categories to logging/categories.h (stickies-v)
f5233f7e98 move-only: Move SourceLocation to util/log.h (stickies-v)
Pull request description:
This is a mostly move-only change. It's a small refactoring that allows logging macros to be used by including a simple `util/log.h` header instead of the full `logging.h` logging implementation. Most of the changes here were cherry-picked from #34374.
Original motivation for this change was to reduce the size and complexity of #34374 (kernel structured logging PR) and reduce the number of conflicts it causes with other PRs. But this should also make sense as a standalone change to have a clearer separation of concerns between log generation and log handling, and avoid needing to depend on the whole logging framework in call sites that only emit log messages.
Recommended to review with `--color-moved=dimmed-zebra --color-moved-ws=ignore-all-space`
ACKs for top commit:
l0rinc:
diff ACK 37cc2a2d95
stickies-v:
re-ACK 37cc2a2d95
optout21:
crACK 37cc2a2d95
sedited:
ACK 37cc2a2d95
Tree-SHA512: c7a761323ae63f07ad290d4e3766ba1348a132c8cc68a9895fa9ae5c89206599c00646c42ef77223ac757b9d8bfe6c181bead15e7058e4d8966b3bac88a8f950
The -blockreservedweight startup option should only affect RPC code,
because IPC clients (currently) do not have a way to signal their intent
to use the node default (the BlockCreateOptions struct defaults
merely document a recommendation for client software).
Before this commit however, if the user set -blockreservedweight
then ApplyArgsManOptions would cause the block_reserved_weight
option passed by IPC clients to be ignored. Users who don't set
this value were not affected.
Fix this by making BlockCreateOptions::block_reserved_weight an
std::optional.
Internal interface users, such as the RPC call sites, don't set a
value so -blockreservedweight is used. Whereas IPC clients do set
a value which is no longer ignored.
Test coverage is added.
mining_basic.py already ensured -blockreservedweight is enforced by
mining RPC methods. This commit adds coverage for Mining interface IPC
clients. It also verifies that -blockreservedweight has no effect on
them.
Co-Authored-By: Russell Yanofsky <russ@yanofsky.org>
d511adb664 [miner] omit dummy extraNonce via IPC (Sjors Provoost)
bf3b5d6d06 test: clarify getCoinbaseRawTx() comparison (Sjors Provoost)
78df9003d6 [doc] Update comments on dummy extraNonces in tests (Anthony Towns)
Pull request description:
This PR changes the Mining IPC interface to stop including a dummy `extraNonce` in the coinbase `scriptSig` by default, exposing only the consensus-required BIP34 height. This simplifies downstream mining software (including Stratum v2), avoids forcing clients to strip or ignore data we generate, and reduces the risk of incompatibilities if future soft forks add required commitments to the `scriptSig`.
Existing behavior is preserved for RPCs, tests, regtest, and internal mining by explicitly opting in to the dummy `extraNonce` where needed (e.g. to satisfy `bad-cb-length` at low heights), so consensus rules and test coverage are unchanged. The remainder of the PR consists of small comment fixes, naming clarifications, and test cleanups to make the intent and behavior clearer.
ACKs for top commit:
achow101:
ACK d511adb664
ryanofsky:
Code review ACK d511adb664. Just rebased since last review and make suggested tweaks. I'd really like to see this PR merged for the cleanups and sanity it brings to this code. Needs another reviewer though.
sedited:
ACK d511adb664
Tree-SHA512: d41fa813eb6b5626f4f475d8abc506b29090f4a2d218f2d6824db58b5ebe2ed7c584a903b44de18ccec142bb79c257b0aba6d6da073f56175aec88df96aaaaba
Preparation for a future commit where kernel's dependency
on logging.cpp is removed completely.
Replace usage of logging\.h with util/log\.h where it
suffices, and fix wrong includes according to iwyu.
3bd98b4508 refactor: use transparent comparator for setBlockIndexCandidates lookups (joaonevess)
Pull request description:
### Rationale
This PR improves code safety by removing a `const_cast` in `src/validation.cpp`.
Currently, `setBlockIndexCandidates` stores mutable `CBlockIndex*`. However, validation logic (like `CVerifyDB`) often holds `const CBlockIndex*`. Previously, checking for existence in the set required casting away constness. While currently benign, this bypasses compiler safety checks and could mask accidental modifications in future refactors.
### Description
1. **Enable Heterogeneous Lookup:** Added `using is_transparent = void;` to `CBlockIndexWorkComparator` in `src/node/blockstorage.h`. This allows the `std::set` to natively accept `const CBlockIndex*` for lookup (utilizing C++14 heterogeneous lookup).
2. **Remove Cast:** Removed the now unnecessary `const_cast<CBlockIndex*>` in `src/validation.cpp`, allowing the compiler to strictly enforce const-correctness.
### Notes
- **Refactoring only:** No behavioral change.
- **Verification:** `validation_tests` and `blockmanager_tests` pass.
ACKs for top commit:
maflcko:
review ACK 3bd98b4508🚪
frankomosh:
ACK 3bd98b4508. Good use of transparent comparator to eliminate `const_cast` in this specific code path.
sedited:
ACK 3bd98b4508
Tree-SHA512: 0f76bdce2a54b759dfec99633afce1e95586e62f4057ecf1e82eed1a073eb8ecb2d659ccbf28a7a139f0aa09a30f058ac6966cafdfbf1f2ee878fa2d86b2c487
fad7bd9ba3 noui: Remove always empty caption while formatting (MarcoFalke)
fa8ebeb332 refactor: [gui] Document that the title is always empty for node message (MarcoFalke)
fafe71b743 refactor: Remove empty caption from ThreadSafeMessageBox (MarcoFalke)
fa8d0088e7 refactor: Remove empty caption from ThreadSafeQuestion (MarcoFalke)
fa0195499c refactor: [gui] Use lambdas over std::bind (MarcoFalke)
eeee1e341f refactor: Remove trailing semicolon after ADD_SIGNALS_DECL_WRAPPER (MarcoFalke)
Pull request description:
Currently, the user interface (noui, gui) has a caption for each message. However, the caption has many issues:
* It is always hard-coded to the empty string.
* This is confusing and tedious when reading or maintaining the code.
* It is redundant, because `noui` will ignore the caption and set the logging prefix (error, warning, info) based on the `style`.
* The gui does prefer to set the title based on the caption, but since it the caption is always empty, the fallback will always be used.
Fix all issues by removing it.
ACKs for top commit:
hebasto:
ACK fad7bd9ba3, I have reviewed the code and it looks OK. Tested on Ubuntu 25.10.
sedited:
ACK fad7bd9ba3
Tree-SHA512: 58ef538b9b3e1cfdcf2955f6de9b8cee335edbf6339723cb693cb4d584817904c962dac5199ee44d7e2860a5332dec1a6abf47e621eb5cf919aa1cdae271b55f
e71c4df168 refactor: replace manual promise with SyncWithValidationInterfaceQueue (ANtutov)
Pull request description:
`BroadcastTransaction()` now waits for validation callbacks using the built-in `validation_signals>SyncWithValidationInterfaceQueue()` instead of creating a local `std::promise` and scheduling a lambda. This removes an unnecessary allocation and uses the canonical API.
ACKs for top commit:
maflcko:
review ACK e71c4df168🌃
rkrux:
lgtm ACK e71c4df168
sedited:
ACK e71c4df168
Tree-SHA512: 602994ba3c2ac91996068aee6eac7e788c3832d7ab949519a9420d2b59e2a67d2d4e67c3c9191ba60e9caa75f1524a95b0851fcd40b6732f6a9956a011b4a120
Previously the coinbase transaction generated by our miner code was
not used downstream, because the getblocktemplate RPC excludes it.
Since the Mining IPC interface was introduced in #30200 we do expose
this dummy coinbase transaction. In Stratum v2 several parts of it
are communicated downstream, including the scriptSig.
This commit removes the dummy extraNonce from the coinbase scriptSig
in block templates requested via IPC. This limits the scriptSig
to what is essential for consensus (BIP34) and removes the need for
external mining software to remove the dummy, or even ignore
the scriptSig we provide and generate it some other way. This
could cause problems if a future soft fork requires additional
data to be committed here.
A test is added to verify the new IPC behavior.
It achieves this by introducing an include_dummy_extranonce
option which defaults to false with all test code updated to
set it to true. Because this option is not exposed via IPC,
callers will no longer see it.
The caller needs to ensure that for blocks 1 through 16
they pad the scriptSig in order to avoid bad-cb-length.
Co-authored-by: Anthony Towns <aj@erisian.com.au>
This allows checking for existence in setBlockIndexCandidates using a const CBlockIndex* without casting away constness, replacing a legacy const_cast check in validation.cpp.
The caption was empty for all call-sites, so this refactor does not
change any behavior.
Note that noui_ThreadSafeMessageBoxRedirect is test-only, so no end-user
behavior is changed here.
There is only one call-site, which provided an empty caption.
Note that noui_ThreadSafeQuestionRedirect is test-only and currently
entrirely unused, so the logging format string change is not a behavior
change.
This refactor does not change any behavior.
48f57bb35b mining: add new getCoinbaseTx() returning a struct (Sjors Provoost)
d59b4cdb57 mining: rename getCoinbaseTx() to ..RawTx() (Sjors Provoost)
Pull request description:
The first commit renames `getCoinbaseTx()` to `getCoinbaseRawTx()` to reflect that it returns a serialised transaction. This does not impact IPC clients, because they do not use the function name.
The second commit then introduces a replacement `getCoinbase()` that provides a struct with everything clients need to construct a coinbase. This avoids clients having to parse and manipulate our dummy transaction.
Deprecate but don't remove `getCoinbaseRawTx()`, `getCoinbaseCommitment()` and `getWitnessCommitmentIndex()`.
After this change we can drop these deprecated methods, which in turn would allow us to clear the dummy transaction from the `getBlock()` result. But that is left for a followup to keep this PR focussed. See https://github.com/Sjors/bitcoin/pull/106 for an approach.
Expand the `interface_ipc.py` functional test to document its usage.
Can be tested using:
- https://github.com/stratum-mining/sv2-tp/pull/59
ACKs for top commit:
ryanofsky:
Code review ACK 48f57bb35b. Just rebased and addressed comments and dropped coinbase tx "template" suffix, which is a nice change
ismaelsadeeq:
code review ACK 48f57bb35b
vasild:
ACK 48f57bb35b
Tree-SHA512: c4f1d752777fb3086a1a0b7b8b06e4205dbe2f3adb41f218855ad1dee952adccc263cf82acd3bf9300cc83c2c64cebd2b27f66a69beee32d325b9a85e3643b0d
8937221304 doc: add release notes for 29415 (Vasil Dimov)
582016fa5f test: add unit test for the private broadcast storage (Vasil Dimov)
e74d54e048 test: add functional test for private broadcast (Vasil Dimov)
818b780a05 rpc: use private broadcast from sendrawtransaction RPC if -privatebroadcast is ON (Vasil Dimov)
eab595f9cf net_processing: retry private broadcast (Vasil Dimov)
37b79f9c39 net_processing: stop private broadcast of a transaction after round-trip (Vasil Dimov)
2de53eee74 net_processing: handle ConnectionType::PRIVATE_BROADCAST connections (Vasil Dimov)
30a9853ad3 net_processing: move a debug check in VERACK processing earlier (Vasil Dimov)
d1092e5d48 net_processing: modernize PushNodeVersion() (Vasil Dimov)
9937a12a2f net_processing: move the debug log about receiving VERSION earlier (Vasil Dimov)
a098f37b9e net_processing: reorder the code that handles the VERSION message (Vasil Dimov)
679ce3a0b8 net_processing: store transactions for private broadcast in PeerManager (Vasil Dimov)
a3faa6f944 node: extend node::TxBroadcast with a 3rd option (Vasil Dimov)
95c051e210 net_processing: rename RelayTransaction() to better describe what it does (Vasil Dimov)
bb49d26032 net: implement opening PRIVATE_BROADCAST connections (Vasil Dimov)
01dad4efe2 net: introduce a new connection type for private broadcast (Vasil Dimov)
94aaa5d31b init: introduce a new option to enable/disable private broadcast (Vasil Dimov)
d6ee490e0a log: introduce a new category for private broadcast (Vasil Dimov)
Pull request description:
_Parts of this PR are isolated in independent smaller PRs to ease review:_
* [x] _https://github.com/bitcoin/bitcoin/pull/29420_
* [x] _https://github.com/bitcoin/bitcoin/pull/33454_
* [x] _https://github.com/bitcoin/bitcoin/pull/33567_
* [x] _https://github.com/bitcoin/bitcoin/pull/33793_
---
To improve privacy, broadcast locally submitted transactions (from the `sendrawtransaction` RPC) to the P2P network only via Tor or I2P short-lived connections, or to IPv4/IPv6 peers but through the Tor network.
* Introduce a new connection type for private broadcast of transactions with the following properties:
* started whenever there are local transactions to be sent
* opened to Tor or I2P peers or IPv4/IPv6 via the Tor proxy
* opened regardless of max connections limits
* after handshake is completed one local transaction is pushed to the peer, `PING` is sent and after receiving `PONG` the connection is closed
* ignore all incoming messages after handshake is completed (except `PONG`)
* Broadcast transactions submitted via `sendrawtransaction` using this new mechanism, to a few peers. Keep doing this until we receive back this transaction from one of our ordinary peers (this takes about 1 second on mainnet).
* The transaction is stored in peerman and does not enter the mempool.
* Once we get an `INV` from one of our ordinary peers, then the normal flow executes: we request the transaction with `GETDATA`, receive it with a `TX` message, put it in our mempool and broadcast it to all our existent connections (as if we see it for the first time).
* After we receive the full transaction as a `TX` message, in reply to our `GETDATA` request, only then consider the transaction has propagated through the network and remove it from the storage in peerman, ending the private broadcast attempts.
The messages exchange should look like this:
```
tx-sender >--- connect -------> tx-recipient
tx-sender >--- VERSION -------> tx-recipient (dummy VERSION with no revealing data)
tx-sender <--- VERSION -------< tx-recipient
tx-sender <--- WTXIDRELAY ----< tx-recipient (maybe)
tx-sender <--- SENDADDRV2 ----< tx-recipient (maybe)
tx-sender <--- SENDTXRCNCL ---< tx-recipient (maybe)
tx-sender <--- VERACK --------< tx-recipient
tx-sender >--- VERACK --------> tx-recipient
tx-sender >--- INV/TX --------> tx-recipient
tx-sender <--- GETDATA/TX ----< tx-recipient
tx-sender >--- TX ------------> tx-recipient
tx-sender >--- PING ----------> tx-recipient
tx-sender <--- PONG ----------< tx-recipient
tx-sender disconnects
```
Whenever a new transaction is received from `sendrawtransaction` RPC, the node will send it to a few (`NUM_PRIVATE_BROADCAST_PER_TX`) recipients right away. If after some time we still have not heard anything about the transaction from the network, then it will be sent to 1 more peer (see `PeerManagerImpl::ReattemptPrivateBroadcast()`).
A few considerations:
* The short-lived private broadcast connections are very cheap and fast wrt network traffic. It is expected that some of those peers could blackhole the transaction. Just one honest/proper peer is enough for successful propagation.
* The peers that receive the transaction could deduce that this is initial transaction broadcast from the transaction originator. This is ok, they can't identify the sender.
---
<details>
<summary>How to test this?</summary>
Thank you, @stratospher and @andrewtoth!
Start `bitcoind` with `-privatebroadcast=1 -debug=privatebroadcast`.
Create a wallet and get a new address, go to the Signet faucet and request some coins to that address:
```bash
build/bin/bitcoin-cli -chain="signet" createwallet test
build/bin/bitcoin-cli -chain="signet" getnewaddress
```
Get a new address for the test transaction recipient:
```bash
build/bin/bitcoin-cli -chain="signet" loadwallet test
new_address=$(build/bin/bitcoin-cli -chain="signet" getnewaddress)
```
Create the transaction:
```bash
# Option 1: `createrawtransaction` and `signrawtransactionwithwallet`:
txid=$(build/bin/bitcoin-cli -chain="signet" listunspent | jq -r '.[0] | .txid')
vout=$(build/bin/bitcoin-cli -chain="signet" listunspent | jq -r '.[0] | .vout')
echo "txid: $txid"
echo "vout: $vout"
tx=$(build/bin/bitcoin-cli -chain="signet" createrawtransaction "[{\"txid\": \"$txid\", \"vout\": $vout}]" "[{\"$new_address\": 0.00001000}]" 0 false)
echo "tx: $tx"
signed_tx=$(build/bin/bitcoin-cli -chain="signet" signrawtransactionwithwallet "$tx" | jq -r '.hex')
echo "signed_tx: $signed_tx"
# OR Option 2: `walletcreatefundedpsbt` and `walletprocesspsbt`:
# This makes it not have to worry about inputs and also automatically sends back change to the wallet.
# Start `bitcoind` with `-fallbackfee=0.00003000` for instance for 3 sat/vbyte fee.
psbt=$(build/bin/bitcoin-cli -chain="signet" walletcreatefundedpsbt "[]" "[{\"$new_address\": 0.00001000}]" | jq -r '.psbt')
echo "psbt: $psbt"
signed_tx=$(build/bin/bitcoin-cli -chain="signet" walletprocesspsbt "$psbt" | jq -r '.hex')
echo "signed_tx: $signed_tx"
```
Finally, send the transaction:
```bash
raw_tx=$(build/bin/bitcoin-cli -chain="signet" sendrawtransaction "$signed_tx")
echo "raw_tx: $raw_tx"
```
</details>
---
<details>
<summary>High-level explanation of the commits</summary>
* New logging category and config option to enable private broadcast
* `log: introduce a new category for private broadcast`
* `init: introduce a new option to enable/disable private broadcast`
* Implement the private broadcast connection handling on the `CConnman` side:
* `net: introduce a new connection type for private broadcast`
* `net: implement opening PRIVATE_BROADCAST connections`
* Prepare `BroadcastTransaction()` for private broadcast requests:
* `net_processing: rename RelayTransaction to better describe what it does`
* `node: extend node::TxBroadcast with a 3rd option`
* `net_processing: store transactions for private broadcast in PeerManager`
* Implement the private broadcast connection handling on the `PeerManager` side:
* `net_processing: reorder the code that handles the VERSION message`
* `net_processing: move the debug log about receiving VERSION earlier`
* `net_processing: modernize PushNodeVersion()`
* `net_processing: move a debug check in VERACK processing earlier`
* `net_processing: handle ConnectionType::PRIVATE_BROADCAST connections`
* `net_processing: stop private broadcast of a transaction after round-trip`
* `net_processing: retry private broadcast`
* Engage the new functionality from `sendrawtransaction`:
* `rpc: use private broadcast from sendrawtransaction RPC if -privatebroadcast is ON`
* New tests:
* `test: add functional test for private broadcast`
* `test: add unit test for the private broadcast storage`
</details>
---
**This PR would resolve the following issues:**
https://github.com/bitcoin/bitcoin/issues/3828 Clients leak IPs if they are recipients of a transaction
https://github.com/bitcoin/bitcoin/issues/14692 Can't configure bitocoind to only send tx via Tor but receive clearnet transactions
https://github.com/bitcoin/bitcoin/issues/19042 Tor-only transaction broadcast onlynet=onion alternative
https://github.com/bitcoin/bitcoin/issues/24557 Option for receive events with all networks, but send transactions and/or blocks only with anonymous network[s]?
https://github.com/bitcoin/bitcoin/issues/25450 Ability to broadcast wallet transactions only via dedicated oneshot Tor connections
https://github.com/bitcoin/bitcoin/issues/32235 Tor: TX circuit isolation
**Issues that are related, but (maybe?) not to be resolved by this PR:**
https://github.com/bitcoin/bitcoin/issues/21876 Broadcast a transaction to specific nodes
https://github.com/bitcoin/bitcoin/issues/28636 new RPC: sendrawtransactiontopeer
---
Further extensions:
* Have the wallet do the private broadcast as well, https://github.com/bitcoin/bitcoin/issues/11887 would have to be resolved.
* Have the `submitpackage` RPC do the private broadcast as well, [draft diff in the comment below](https://github.com/bitcoin/bitcoin/pull/29415#pullrequestreview-2972293733), thanks ismaelsadeeq!
* Add some stats via RPC, so that the user can better monitor what is going on during and after the broadcast. Currently this can be done via the debug log, but that is not convenient.
* Make the private broadcast storage, currently in peerman, persistent over node restarts.
* Add (optional) random delay before starting to broadcast the transaction in order to avoid correlating unrelated transactions based on the time when they were broadcast. Suggested independently of this PR [here](https://github.com/bitcoin/bitcoin/issues/30471).
* Consider periodically sending transactions that did not originate from the node as decoy, discussed [here](https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2035414972).
* Consider waiting for peer's FEEFILTER message and if the transaction that was sent to the peer is below that threshold, then assume the peer is going to drop it. Then use this knowledge to retry more aggressively with another peer, instead of the current 10 min. See [comment below](https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-3258611648).
* It may make sense to be able to override the default policy -- eg so submitrawtransaction can go straight to the mempool and relay, even if txs are normally privately relayed. See [comment below](https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-3427086681).
* As a side effect we have a new metric available - the time it takes for a transaction to reach a random node in the network (from the point of view of the private broadcast recipient the tx originator is a random node somewhere in the network). This can be useful for monitoring, unrelated to privacy characteristics of this feature.
---
_A previous incarnation of this can be found at https://github.com/bitcoin/bitcoin/pull/27509. It puts the transaction in the mempool and (tries to) hide it from the outside observers. This turned out to be too error prone or maybe even impossible._
ACKs for top commit:
l0rinc:
code review diff ACK 8937221304
andrewtoth:
ACK 8937221304
pinheadmz:
ACK 8937221304
w0xlt:
ACK 8937221304 with nit https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2654849875
mzumsande:
re-ACK 8937221304
Tree-SHA512: d51dadc865c2eb080c903cbe2f669e69a967e5f9fc64e9a20a68f39a67bf0db6ac2ad682af7fa24ef9f0942a41c89959341a16ba7b616475e1c5ab8e563b9b96
Introduce a new method intended to replace getCoinbaseRawTx(), which
provides a struct with everything clients need to construct a coinbase.
This is safer than providing a raw dummy coinbase that clients then have
to manipulate.
The CoinbaseTx data is populated during the dummy transaction generation
and stored in struct CBlockTemplate.
Expand the interface_ipc.py functional test to document its usage
and ensure equivalence.
d7de5b109f logs: show reindex progress in `ImportBlocks` (Lőrinc)
Pull request description:
### Summary
When triggering a reindex, users have no indication of progress.
### Fix
This patch precomputes the total number of block files so progress can be shown.
Instead of only displaying which block file is being processed, it now shows the percent complete.
### Reproducer + expected results
```bash
cmake -B build -DCMAKE_BUILD_TYPE=Release && make -C build -j && ./build/bin/bitcoind -datadir=demo -reindex
```
Before, the block files were shown one-by-one, there's no way to see how much work is left:
```
Reindexing block file blk00000.dat...
Loaded 119920 blocks from external file in 1228ms
Reindexing block file blk00001.dat...
Loaded 10671 blocks from external file in 284ms
Reindexing block file blk00002.dat...
Loaded 5459 blocks from external file in 263ms
Reindexing block file blk00003.dat...
Loaded 5595 blocks from external file in 267ms
```
After the change we add a percentage:
```
Reindexing block file blk00000.dat (0% complete)...
Loaded 119920 blocks from external file in 1255ms
Reindexing block file blk00001.dat (1% complete)...
Loaded 10671 blocks from external file in 303ms
Reindexing block file blk00002.dat (2% complete)...
Loaded 5459 blocks from external file in 278ms
Reindexing block file blk00003.dat (3% complete)...
Loaded 5595 blocks from external file in 285ms
```
ACKs for top commit:
enirox001:
Concept ACK d7de5b1
rkrux:
lgtm ACK d7de5b109f
danielabrozzoni:
tACK d7de5b109f - code reviewed and tested on my archival node.
maflcko:
review ACK d7de5b109f💇
Tree-SHA512: 359a539b781ad8b73e2a616c951567062a76be27cf90e5b88bb5309295af9cd7994e327f185bacc1482b43b892b38329593b4043a5e71d8800e3e4b7a3954310
Specifically gets rid of batchpriority, chainparams, script/sign.h and
system includes.
Also take the opportunity of cleaning up the headers for the effected
files and adding them to the iwyu-enforced set.
fa4cb13b52 test: [doc] Manually unify stale headers (MarcoFalke)
fa5f297748 scripted-diff: [doc] Unify stale copyright headers (MarcoFalke)
Pull request description:
Historically, the upper year range in file headers was bumped manually
or with a script.
This has many issues:
* The script is causing churn. See for example commit 306ccd4, or
drive-by first-time contributions bumping them one-by-one. (A few from
this year: https://github.com/bitcoin/bitcoin/pull/32008,
https://github.com/bitcoin/bitcoin/pull/31642,
https://github.com/bitcoin/bitcoin/pull/32963, ...)
* Some, or likely most, upper year values were wrong. Reasons for
incorrect dates could be code moves, cherry-picks, or simply bugs in
the script.
* The upper range is not needed for anything.
* Anyone who wants to find the initial file creation date, or file
history, can use `git log` or `git blame` to get more accurate
results.
* Many places are already using the `-present` suffix, with the meaning
that the upper range is omitted.
To fix all issues, this bumps the upper range of the copyright headers
to `-present`.
Further notes:
* Obviously, the yearly 4-line bump commit for the build system (c.f.
b537a2c02a) is fine and will remain.
* For new code, the date range can be fully omitted, as it is done
already by some developers. Obviously, developers are free to pick
whatever style they want. One can list the commits for each style.
* For example, to list all commits that use `-present`:
`git log --format='%an (%ae) [%h: %s]' -S 'present The Bitcoin'`.
* Alternatively, to list all commits that use no range at all:
`git log --format='%an (%ae) [%h: %s]' -S '(c) The Bitcoin'`.
<!--
* The lower range can be wrong as well, so it could be omitted as well,
but this is left for a follow-up. A previous attempt was in
https://github.com/bitcoin/bitcoin/pull/26817.
ACKs for top commit:
l0rinc:
ACK fa4cb13b52
rkrux:
re-ACK fa4cb13b52
janb84:
ACK fa4cb13b52
Tree-SHA512: e5132781bdc4417d1e2922809b27ef4cf0abb37ffb68c65aab8a5391d3c917b61a18928ec2ec2c75ef5184cb79a5b8c8290d63e949220dbeab3bd2c0dfbdc4c5
This frees up the name getCoinbaseTx() for the next commit.
Changing a function name does not impact IPC clients, as they only
consider the function signature and sequence number.
db2d39f642 fuzz: add subtest for re-downloading a previously pruned block (Eugene Siegel)
45f5b2dac3 fuzz: Add fuzzer for block index (Martin Zumsande)
c011e3aa54 test: Wrap validation functions with TestChainstateManager (Martin Zumsande)
Pull request description:
This adds a fuzz target for the block index and various events in validation that interact with it.
It can create arbitrary tree-like structure of block indexes, simulating (so far) the following events:
- Adding a header
- Receiving the full block (may be valid or not)
- `ActivateBestChain()` - Reorging the chain to a new chain tip (possibly encountering invalid blocks on the way)
- Pruning a block in the best chain
- Receiving a previously pruned block again (`getblockfrompeer`)
It might be interesting / possible to extend this to more events, such as dealing with more than one chainstate (assumeutxo).
The test skips all actual validation of header/ block / transaction data by just simulating the outcome, and also doesn't interact with the data directory.
The main goal is to ensure the integrity of the block index tree in all fuzzed constellations, by calling `CheckBlockIndex()` at the end of each iteration.
Compared to #29158 this approach has a more limited scope (by skipping all actual validation), but it is fast - it doesn't do a full init sequence on each iteration, but "cleans up" after itself by resetting the global validation state after each iteration.
ACKs for top commit:
Crypt-iQ:
reACK db2d39f642
maflcko:
review ACK db2d39f642🍶
sedited:
Re-ACK db2d39f642
Tree-SHA512: 76cd5f8f4d7d7258620b46d7438bad4508c3bdc98825b48b60f694b5a9838e2b2cf4967c0ead181f86f66f4939ddfe552471851b9d18f84f584c03dd7e09fc43
d9319b06cf refactor: unify container presence checks - non-trivial counts (Lőrinc)
039307554e refactor: unify container presence checks - trivial counts (Lőrinc)
8bb9219b63 refactor: unify container presence checks - find (Lőrinc)
Pull request description:
### Summary
Instead of counting occurrences in sets and maps, the C++20 `::contains` method expresses the intent unambiguously and can return early on first encounter.
### Context
Applied clang‑tidy's [readability‑container‑contains](https://clang.llvm.org/extra/clang-tidy/checks/readability/container-contains.html) check, though many cases required manual changes since tidy couldn't fix them automatically.
### Changes
The changes made here were:
| From | To |
|------------------------|------------------|
| `m.find(k) == m.end()` | `!m.contains(k)` |
| `m.find(k) != m.end()` | `m.contains(k)` |
| `m.count(k)` | `m.contains(k)` |
| `!m.count(k)` | `!m.contains(k)` |
| `m.count(k) == 0` | `!m.contains(k)` |
| `m.count(k) != 1` | `!m.contains(k)` |
| `m.count(k) == 1` | `m.contains(k)` |
| `m.count(k) < 1` | `!m.contains(k)` |
| `m.count(k) > 0` | `m.contains(k)` |
| `m.count(k) != 0` | `m.contains(k)` |
> Note that `== 1`/`!= 1`/`< 1` only apply to simple [maps](https://en.cppreference.com/w/cpp/container/map/contains)/[sets](https://en.cppreference.com/w/cpp/container/set/contains) and had to be changed manually.
There are many other cases that could have been changed, but we've reverted most of those to reduce conflict with other open PRs.
-----
<details>
<summary>clang-tidy command on Mac</summary>
```bash
rm -rfd build && \
cmake -B build \
-DCMAKE_C_COMPILER="$(brew --prefix llvm)/bin/clang" \
-DCMAKE_CXX_COMPILER="$(brew --prefix llvm)/bin/clang++" \
-DCMAKE_OSX_SYSROOT="$(xcrun --show-sdk-path)" \
-DCMAKE_C_FLAGS="-target arm64-apple-macos11" \
-DCMAKE_CXX_FLAGS="-target arm64-apple-macos11" \
-DCMAKE_EXPORT_COMPILE_COMMANDS=ON -DBUILD_BENCH=ON -DBUILD_FUZZ_BINARY=ON -DBUILD_FOR_FUZZING=ON
"$(brew --prefix llvm)/bin/run-clang-tidy" -quiet -p build -j$(nproc) -checks='-*,readability-container-contains' | grep -v 'clang-tidy'
```
</details>
Note: this is a take 2 of https://github.com/bitcoin/bitcoin/pull/33094 with fewer contentious changes.
ACKs for top commit:
optout21:
reACK d9319b06cf
sedited:
ACK d9319b06cf
janb84:
re ACK d9319b06cf
pablomartin4btc:
re-ACK d9319b06cf
ryanofsky:
Code review ACK d9319b06cf. I manually reviewed the full change, and it seems there are a lot of positive comments about this and no more very significant conflicts, so I will merge it shortly.
Tree-SHA512: e4415221676cfb88413ccc446e5f4369df7a55b6642347277667b973f515c3c8ee5bfa9ee0022479c8de945c89fbc9ff61bd8ba086e70f30298cbc1762610fe1
59b93f11e8 rest: print also HTTP response reason in case of an error (Roman Zeyde)
7fe94a0493 rest: add a test for unsuported `/blockpart/` request type (Roman Zeyde)
55d0d19b5c rest: deduplicate `interface_rest.py` negative tests (Roman Zeyde)
89eb531024 rest: update release notes for `/blockpart/` endpoint (Roman Zeyde)
41118e17f8 blockstorage: simplify partial block read validation (Roman Zeyde)
599effdeab rest: reformat `uri_prefixes` initializer list (Roman Zeyde)
Pull request description:
The commits below should resolve a few leftovers from #33657.
ACKs for top commit:
l0rinc:
ACK 59b93f11e8
hodlinator:
re-ACK 59b93f11e8
Tree-SHA512: ae45e08edd315018e11283b354fb32f9658f5829c956554dc662a81c2e16397def7c3700e6354e0a91ff03c850def35638a69ec2668b7c015d25d6fed42b92bb
Extend `PeerManager` with a transaction storage and a new method
`InitiateTxBroadcastPrivate()` which:
* adds a transaction to that storage and
* calls `CConnman::PrivateBroadcast::NumToOpenAdd()` to open dedicated
privacy connections that will pick an entry from the transaction
storage and broadcast it.
Extend `node::TxBroadcast` with a 3rd option to not add the
transaction to the mempool and broadcast privately.
This is a non-functional change - `BroadcastTransaction()` will not
do anything if the 3rd options is passed and is not used by any of
its callers.
Rename `PeerManager::RelayTransaction()` to
`PeerManager::InitiateTxBroadcastToAll()`. The transaction is not
relayed when the method returns. It is only enqueued for a possible
broadcasting at a later time. Also, there will be another method which
only does so to Tor or I2P peers.
82be652e40 doc: Improve ChainstateManager documentation, use consistent terms (Ryan Ofsky)
af455dcb39 refactor: Simplify pruning functions (TheCharlatan)
ae85c495f1 refactor: Delete ChainstateManager::GetAll() method (Ryan Ofsky)
6a572dbda9 refactor: Add ChainstateManager::ActivateBestChains() method (Ryan Ofsky)
491d827d52 refactor: Add ChainstateManager::m_chainstates member (Ryan Ofsky)
e514fe6116 refactor: Delete ChainstateManager::SnapshotBlockhash() method (Ryan Ofsky)
ee35250683 refactor: Delete ChainstateManager::IsSnapshotValidated() method (Ryan Ofsky)
d9e82299fc refactor: Delete ChainstateManager::IsSnapshotActive() method (Ryan Ofsky)
4dfe383912 refactor: Convert ChainstateRole enum to struct (Ryan Ofsky)
352ad27fc1 refactor: Add ChainstateManager::ValidatedChainstate() method (Ryan Ofsky)
a229cb9477 refactor: Add ChainstateManager::CurrentChainstate() method (Ryan Ofsky)
a9b7f5614c refactor: Add Chainstate::StoragePath() method (Ryan Ofsky)
840bd2ef23 refactor: Pass chainstate parameters to MaybeCompleteSnapshotValidation (Ryan Ofsky)
1598a15aed refactor: Deduplicate Chainstate activation code (Ryan Ofsky)
9fe927b6d6 refactor: Add Chainstate m_assumeutxo and m_target_utxohash members (Ryan Ofsky)
6082c84713 refactor: Add Chainstate::m_target_blockhash member (Ryan Ofsky)
de00e87548 test: Fix broken chainstatemanager_snapshot_init check (Ryan Ofsky)
Pull request description:
This PR contains the first part of #28608, which tries to make assumeutxo code more maintainable, and improve it by not locking `cs_main` for a long time when the snapshot block is connected, and by deleting the snapshot validation chainstate when it is no longer used, instead of waiting until the next restart.
The changes in this PR are just refactoring. They make `Chainstate` objects self-contained, so for example, it is possible to determine what blocks to connect to a chainstate without querying `ChainstateManager`, and to determine whether a Chainstate is validated without basing it on inferences like `&cs != &ActiveChainstate()` or `GetAll().size() == 1`.
The PR also tries to make assumeutxo terminology less confusing, using "current chainstate" to refer to the chainstate targeting the current network tip, and "historical chainstate" to refer to the chainstate downloading old blocks and validating the assumeutxo snapshot. It removes uses of the terms "active chainstate," "usable chainstate," "disabled chainstate," "ibd chainstate," and "snapshot chainstate" which are confusing for various reasons.
ACKs for top commit:
maflcko:
re-review ACK 82be652e40🕍
fjahr:
re-ACK 82be652e40
sedited:
Re-ACK 82be652e40
Tree-SHA512: 81c67abba9fc5bb170e32b7bf8a1e4f7b5592315b4ef720be916d5f1f5a7088c0c59cfb697744dd385552f58aa31ee36176bae6a6e465723e65861089a1252e5
