525e9dcba0b8c6744bcd3725864f39786afc8ed5 Add submitSolution to BlockTemplate interface (Sjors Provoost)
47b4875ef050c4a41bd04398021af8d605415cab Add getCoinbaseMerklePath() to Mining interface (Sjors Provoost)
63d6ad7c89cd682f6e779968c4861ea085058356 Move BlockMerkleBranch back to merkle.{h,cpp} (Sjors Provoost)
Pull request description:
The new `BlockTemplate` interface introduced in #30440 allows for a more efficient way for a miner to submit the block solution. Instead of having the send the full block, it only needs to provide the nonce, timestamp, version fields and coinbase transaction.
This PR introduces `submitSolution()` for that. It's currently unused.
#29432 and https://github.com/Sjors/bitcoin/pull/48 use it to process the Stratum v2 message [SubmitSolution](https://github.com/stratum-mining/sv2-spec/blob/main/07-Template-Distribution-Protocol.md#77-submitsolution-client---server). The method should be sufficiently generic to work with alternative mining protocols (none exist that I'm aware off).
This PR also introduces `getCoinbaseMerklePath()`, which is needed in Stratum v2 to construct the `merkle_path` field of the `NewTemplate` message (see [spec](https://github.com/stratum-mining/sv2-spec/blob/main/07-Template-Distribution-Protocol.md#72-newtemplate-server---client)). The coinbase merkle path is also used in Stratum "v1", see e.g. https://bitcoin.stackexchange.com/questions/109820/questions-on-merkle-root-hashing-for-stratum-pools
This last function uses `BlockMerkleBranch` which was moved to the test code in #13191. The reason back then for moving it was that it was no longer used. This PR moves it back.
This PR does not change behaviour since both methods are unused.
ACKs for top commit:
achow101:
ACK 525e9dcba0b8c6744bcd3725864f39786afc8ed5
itornaza:
Code review ACK 525e9dcba0b8c6744bcd3725864f39786afc8ed5
tdb3:
Code review and light test ACK 525e9dcba0b8c6744bcd3725864f39786afc8ed5
ryanofsky:
Code review ACK 525e9dcba0b8c6744bcd3725864f39786afc8ed5. Left minor suggestions but none are important, and looks like this could be merged as-is
Tree-SHA512: 2a6a8f5d409ff4926643193cb67702240c7c687615414371e53383d2c13c485807f65e21e8ed98515b5456eca3d9fca13cec04675814a4081467d88b849c5653
fa22e5c430acaef9713d9a4b4b97bb3f4876f816 refactor: Remove dead code that assumed tip == nullptr (MarcoFalke)
fa2e4439652172df27f14508eef078bd0ee2fca5 refactor: Replace g_genesis_wait_cv with m_tip_block_cv (MarcoFalke)
fa7f52af1a47ebe3d1bc00e3bac70bf9f6fa769b refactor: Use wait_for predicate to check for interrupt (MarcoFalke)
5ca28ef28bcca1775ff49921fc2528d9439b71ab refactor: Split up NodeContext shutdown_signal and shutdown_request (Ryan Ofsky)
fad8e7fba7bf2c523a191309d392cab79668264b bugfix: Mark m_tip_block_cv as guarded by m_tip_block_mutex (MarcoFalke)
fa18586c29d4faba3d3f478eaabf9c4cbc1a16e2 refactor: Add missing GUARDED_BY(m_tip_block_mutex) (MarcoFalke)
fa4c0750331f36121ba92bbc2f22c615b7934e52 doc: Clarify waitTipChanged docs (MarcoFalke)
Pull request description:
`g_genesis_wait_cv` is similar to `m_tip_block_cv` but shuffling everything through a redundant `boost::signals2`.
So remove it, along with some other dead code, as well as minor fixups.
ACKs for top commit:
ryanofsky:
Code review ACK fa22e5c430acaef9713d9a4b4b97bb3f4876f816 (just rebased since last review)
Sjors:
ACK fa22e5c430acaef9713d9a4b4b97bb3f4876f816
TheCharlatan:
ACK fa22e5c430acaef9713d9a4b4b97bb3f4876f816
Tree-SHA512: a2cb59b651aaf85a3574723adfe403487566788ad945933b0458816ccc841fce08ca77b31afbd2d6adb5bf1deed7229c028bee74fb4bbaf6576e9edcfa0ad817
This has been unused since #29648.
Noticed while running a newer version of clang-tidy (19.1.1):
```bash
[127/391][6.2s] /opt/homebrew/opt/llvm/bin/clang-tidy -p=build -quiet --config-file=/bitcoin/src/.clang-tidy /bitcoin/src/test/script_tests.cpp
bitcoin/src/test/script_tests.cpp:126:25: error: local copy 'tx2' of the variable 'tx' is never modified and never used; consider removing the statement [performance-unnecessary-copy-initialization,-warnings-as-errors]
126 | CMutableTransaction tx2 = tx;
| ~~~~~~~~~~~~~~~~~~~~^~~~~~~~~
127 | BOOST_CHECK_MESSAGE(VerifyScript(scriptSig, scriptPubKey, &scriptWitness, flags, MutableTransactionSignatureChecker(&tx, 0, txCredit.vout[0].nValue, MissingDataBehavior::ASSERT_FAIL), &err) == expect, message);
512 warnings generated.
```
98c1536852d1de9a978b11046e7414e79ed40b46 test: add getorphantxs tests (tdb3)
93f48fceb7dd332ef980ce890ff7750b995d6077 test: add tx_in_orphanage() (tdb3)
34a9c10e8cdb3e9cd40fc3a420df8f73e0208a48 rpc: add getorphantxs (tdb3)
f511ff3654d999951a64098c8a9f2f8e29771dad refactor: move verbosity parsing to rpc/util (tdb3)
532491faf1aa90053af52cbedce403b9eccf0bc3 net: add GetOrphanTransactions() to PeerManager (tdb3)
91b65adff2aaf16f42c5ccca6e16b951e0e84f9a refactor: add OrphanTxBase for external use (tdb3)
Pull request description:
This PR adds a new hidden rpc, `getorphantxs`, that provides the caller with a list of orphan transactions. This rpc may be helpful when checking orphan behavior/scenarios (e.g. in tests like `p2p_orphan_handling`) or providing additional data for statistics/visualization.
```
getorphantxs ( verbosity )
Shows transactions in the tx orphanage.
EXPERIMENTAL warning: this call may be changed in future releases.
Arguments:
1. verbosity (numeric, optional, default=0) 0 for an array of txids (may contain duplicates), 1 for an array of objects with tx details, and 2 for details from (1) and tx hex
Result (for verbose = 0):
[ (json array)
"hex", (string) The transaction hash in hex
...
]
Result (for verbose = 1):
[ (json array)
{ (json object)
"txid" : "hex", (string) The transaction hash in hex
"wtxid" : "hex", (string) The transaction witness hash in hex
"bytes" : n, (numeric) The serialized transaction size in bytes
"vsize" : n, (numeric) The virtual transaction size as defined in BIP 141. This is different from actual serialized size for witness transactions as witness data is discounted.
"weight" : n, (numeric) The transaction weight as defined in BIP 141.
"expiration" : xxx, (numeric) The orphan expiration time expressed in UNIX epoch time
"from" : [ (json array)
n, (numeric) Peer ID
...
]
},
...
]
Result (for verbose = 2):
[ (json array)
{ (json object)
"txid" : "hex", (string) The transaction hash in hex
"wtxid" : "hex", (string) The transaction witness hash in hex
"bytes" : n, (numeric) The serialized transaction size in bytes
"vsize" : n, (numeric) The virtual transaction size as defined in BIP 141. This is different from actual serialized size for witness transactions as witness data is discounted.
"weight" : n, (numeric) The transaction weight as defined in BIP 141.
"expiration" : xxx, (numeric) The orphan expiration time expressed in UNIX epoch time
"from" : [ (json array)
n, (numeric) Peer ID
...
],
"hex" : "hex" (string) The serialized, hex-encoded transaction data
},
...
]
Examples:
> bitcoin-cli getorphantxs 2
> curl --user myusername --data-binary '{"jsonrpc": "2.0", "id": "curltest", "method": "getorphantxs", "params": [2]}' -H 'content-type: application/json' http://127.0.0.1:8332/
```
```
$ build/src/bitcoin-cli getorphantxs 2
[
{
"txid": "50128aac5deab548228d74d846675ad4def91cd92453d81a2daa778df12a63f2",
"wtxid": "bb61659336f59fcf23acb47c05dc4bbea63ab533a98c412f3a12cb813308d52c",
"bytes": 133,
"vsize": 104,
"weight": 415,
"expiration": 1725663854,
"from": [
1
],
"hex": "020000000001010b992959eaa2018bbf31a4a3f9aa30896a8144dbd5cfaf263bf07c0845a3a6620000000000000000000140fe042a010000002251202913b252fe537830f843bfdc5fa7d20ba48639a87c86ff837b92d083c55ad7c102015121c0000000000000000000000000000000000000000000000000000000000000000100000000"
},
{
"txid": "330bb7f701604a40ade20aa129e9a3eb8a7bf024e599084ca1026d3222b9f8a1",
"wtxid": "b7651f7d4c1a40c4d01f6a1e43a121967091fa0f56bb460146c1c5c068e824f6",
"bytes": 133,
"vsize": 104,
"weight": 415,
"expiration": 1725663854,
"from": [
2
],
"hex": "020000000001013600adfe41e0ebd2454838963d270916d2b47239c9eebb93a992b720d3589a080000000000000000000140fe042a010000002251202913b252fe537830f843bfdc5fa7d20ba48639a87c86ff837b92d083c55ad7c102015121c0000000000000000000000000000000000000000000000000000000000000000100000000"
}
]
```
ACKs for top commit:
glozow:
reACK 98c1536852d
hodlinator:
re-ACK 98c1536852d1de9a978b11046e7414e79ed40b46
danielabrozzoni:
ACK 98c1536852d1de9a978b11046e7414e79ed40b46
pablomartin4btc:
tACK 98c1536852d1de9a978b11046e7414e79ed40b46
itornaza:
reACK 98c1536852d1de9a978b11046e7414e79ed40b46
Tree-SHA512: 66075f9faa83748350b87397302100d08af92cbef5fadb27f2b4903f028c08020bf34a23e17262b41abb3f379ca9f46cf6cd5459b8681f2b83bffbbaf3c03ff9
a7498cc7e26b4b3de976e111de2bd2d79b056b31 Fix bug in p2p_headers_presync harness (marcofleon)
Pull request description:
The calculation for the test chain's work (`total_work`) should be outside of the loop. Previously, `total_work` was being miscalculated due to multiple additions of work from the same headers. Now, each header's work is only counted once, providing an accurate total.
https://github.com/bitcoin/bitcoin/pull/30918 followup
ACKs for top commit:
dergoegge:
utACK a7498cc7e26b4b3de976e111de2bd2d79b056b31
instagibbs:
ACK a7498cc7e26b4b3de976e111de2bd2d79b056b31
glozow:
makes sense, utACK a7498cc7e26b4b3de976e111de2bd2d79b056b31
mzumsande:
ACK a7498cc7e2
Tree-SHA512: b95f25dcf7ace220e30f1d72f50d85ee18777467927c0cc1ed8582b390cb7185ffc0e2f127309eb083044fb41f5a13fce5ebb15b7952718a899bafff26921be8
Instead of having a single NodeContext::shutdown member that is used both to
request shutdowns and check if they have been requested, use separate members
for each. Benefits of this change:
1. Should make code a little clearer and easier to search because it is easier
to see which parts of code are triggering shutdowns and which parts are just
checking to see if they were triggered.
2. Makes it possible for init.cpp to specify additional code to run when a
shutdown is requested, like signalling the m_tip_block_cv condition variable.
Motivation for this change was to remove hacky NodeContext argument and
m_tip_block_cv access from the StopRPC function, so StopRPC can just be
concerned with RPC functionality, not other node functionality.
5c7cacf649a6b474b876a7d219c7dc683a25e33d ci: Remove natpmp build option and libnatpmp dependency (laanwj)
7e7ec984da50f45491b994aaab180e7735ad1d8f doc: Remove mention of natpmp build options (laanwj)
061c3e32a26c6c04bf734d62627403758d7e51d9 depends: Drop natpmp and associated option from depends (laanwj)
20a18bf6aa38e87f72e2645482d00d0c77a344f5 build: Drop libnatpmp from build system (laanwj)
7b04709862f48e9020c7bef79cb31dd794cf91d0 qt: Changes for built-in PCP+NAT-PMP (laanwj)
52f8ef66c61b82457a161f3b90cc87f57d1dda80 net: Replace libnatpmp with built-in NATPMP+PCP implementation in mapport (laanwj)
97c97177cdb2f596aa7d4a65c4bde87de50a96f2 net: Add PCP and NATPMP implementation (laanwj)
d72df63d16941576b3523cfeaa49985cf3cd4d42 net: Use GetLocalAddresses in Discover (laanwj)
e02030432b77abbf27bb4f67d879d3ad6d6366e6 net: Add netif utility (laanwj)
754e4254388ec8ac1be6cf807bf300cd43fd3da5 crypto: Add missing WriteBE16 function (laanwj)
Pull request description:
Continues #30005. Closes #17012..
This PR adds PCP (Port Control Protocol) from [RFC6887](https://datatracker.ietf.org/doc/html/rfc6887). This adds, in addition to the existing IPv4 port mapping (which now uses PCP, with fallback to NAT-PMP), support for IPv6 pinholing-that is, opening a port on the firewall to make it reachable.
PCP, like NAT-PMP is a simple UDP-based protocol, and the implementation is self-contained, so this gets rid of lthe libnatpnp dependency without adding a new one. It should otherwise be a drop-in replacement. NAT-PMP fallback is implemented so this will not make router support worse.
For now it is disabled by default, though in the future (not in this PR) we could consider enable it by default to increase the number of connectable nodes without adding significant attack surface.
To test:
```bash
bitcoind -regtest -natpmp=1 -debug=net
```
(most of the changes in this PR are, ironically, removing the libnatpmp dependency and associated build system and build docs)
## TODO
- [x] Default gateway discovery on Linux / FreeBSD
- [x] Default gateway discovery on Windows
- [x] Default gateway discovery on MacOS
- [x] Either solve FreeBSD compile issue (probably upstream issue) or remove FreeBSD support
## Things to consider for follow-up PRs
- https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1658764974 avoid unreachable nets (not given to -onlynet=)
- https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1658949236 could announce an addr:port where we do not listen (no -bind)
- https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1684368824 could announce the wrong port because it uses GetListenPort()
- https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1679709347 if we requested one port but another was assigned, then which one to use in the renewal?
- https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1772017020 Use `GetAdapterAddresses` to discover local addresses for Windows
ACKs for top commit:
Sjors:
ACK 5c7cacf649a6b474b876a7d219c7dc683a25e33d
achow101:
ACK 5c7cacf649a6b474b876a7d219c7dc683a25e33d
vasild:
ACK 5c7cacf649a6b474b876a7d219c7dc683a25e33d
Tree-SHA512: e35b69e56d5f5449a3d48a799f9b7b65107c65eeb3e245c2c1e9d42221e469ca5ead90afae423160601cd664dd553a51c859e04f4492f335b064aae3bf23e3bc
6c3c619b35cc03e883f9d2b3326f906aedde9ba7 test: generalize HasReason and use it in FailFmtWithError (Lőrinc)
Pull request description:
Standardized boost exception checking in recent tests introduced in https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1756493521 by extending `HasReason` to accept `const char*` through `string_view` in `operator()`.
Note that `HasReason` only checks partial matches - but since we're specifying the whole error string, it doesn't affect us in this case.
ACKs for top commit:
maflcko:
review ACK 6c3c619b35cc03e883f9d2b3326f906aedde9ba7
hodlinator:
ACK 6c3c619b35cc03e883f9d2b3326f906aedde9ba7
Tree-SHA512: 740fb18b8fea78e4eb9740ceb0fe75d37246c28cfa2638b9d093e9514dd6d7926cc5be9ec57f8027cca3aa9d616e8c54322d2401cfa67fd25282f7816e63532d
The Mining interface uses this function in the next commit
to calculate the coinbase merkle path. Stratum v2 uses
this to send a compact work template.
This partially undoes the change in 4defdfab94504018f822dc34a313ad26cedc8255,
but is not a revert, because the implementation changed in the meantime.
This commit also documents the function.
1a332817665f77f55090fa166930fec0e5500727 doc: multiprocess documentation improvements (Ryan Ofsky)
d043950ba245cdcd09dc389a71d43b0a58e41a8b multiprocess: Add serialization code for BlockValidationState (Ryan Ofsky)
33c2eee285e35db50b33efb6e782ff002f9d18ec multiprocess: Add IPC wrapper for Mining interface (Ryan Ofsky)
06882f84017f6b569b46a644f39b6d3c120ec6cf multiprocess: Add serialization code for vector<char> (Russell Yanofsky)
095286f790acda4a32f04c77aa86106007e2a0d8 multiprocess: Add serialization code for CTransaction (Russell Yanofsky)
69dfeb18761cfd933b8a9a6e69ce9d3c516ba949 multiprocess: update common-types.h to use C++20 concepts (Ryan Ofsky)
206c6e78eec7c71d317666a1af07acf357ba001e build: Make bitcoin_ipc_test depend on bitcoin_ipc (Ryan Ofsky)
070e6a32d5ff4be2f4f1f6a8200fba2f61df16e3 depends: Update libmultiprocess library for cmake headers target (Ryan Ofsky)
Pull request description:
Add Cap'n Proto wrapper for the Mining interface introduced in #30200, and its associated types.
This PR combined with #30509 will allow a separate mining process, like the one being implemented in https://github.com/Sjors/bitcoin/pull/48, to connect to the node over IPC, and create, manage, and submit block templates. (#30437 shows another simpler demo of a process using the Mining interface.)
---
This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/issues/28722).
ACKs for top commit:
achow101:
ACK 1a332817665f77f55090fa166930fec0e5500727
TheCharlatan:
ACK 1a332817665f77f55090fa166930fec0e5500727
itornaza:
ACK 1a332817665f77f55090fa166930fec0e5500727
Tree-SHA512: 0791078dd6885dbd81e3d14c75fffff3da8d1277873af379ea6f9633e910c11485bb324e4cde3d936d50d343b16a10b0e8fc1e0fc6d7bdca7f522211da50c01e
7bd3ee62f6d6f59ca599e85f81776d282dee1539 test: Use shell builtins in run_command test case (Ava Chow)
Pull request description:
Uses the [suggested command](https://github.com/bitcoin/bitcoin/issues/30938#issuecomment-2363906135)
Fixes#30938
ACKs for top commit:
maflcko:
review ACK 7bd3ee62f6d6f59ca599e85f81776d282dee1539
hebasto:
ACK 7bd3ee62f6d6f59ca599e85f81776d282dee1539.
Tree-SHA512: 683b15cafaf0103eeadf872ea6ce9a7d884b2605d3dcf4e66b0173cdb149c24965e7c5fa62aaddf2ac55df3f449aeb787176992c96cfee5d0b86621259e1dfe9
Add support for passing CTransaction and CTransactionRef types to IPC
functions.
These types can't be passed currently because IPC serialization code currently
only supports deserializing types that have an Unserialize() method, which
CTransaction does not, because it is supposed to represent immutable
transactions. Work around this by adding a CustomReadField overload that will
call CTransaction's deserialize_type constructor.
These types also can't be passed currently because serializing transactions
requires TransactionSerParams to be set. Fix this by setting TX_WITH_WITNESS as
default serialization parameters for IPC code.
7942951e3fcc58f7db0059546d03be9ea243f1db Remove unused g_best_block (Ryan Ofsky)
e3a560ca68d79b056a105a65ed0c174a9631aba9 rpc: use waitTipChanged for longpoll (Ryan Ofsky)
460687a09c2af336fce853d9ffb790d01429eec6 Remove unused CRPCSignals (Sjors Provoost)
dca923150e3ac10a57c23a7e29e76516d32ec10d Replace RPCNotifyBlockChange with waitTipChanged() (Sjors Provoost)
2a40ee1121903847cdd3f6c5b4796e4d5471b2df rpc: check for negative timeout arg in waitfor* (Sjors Provoost)
de7c855b3af99fe6b62279c5c2d08888a5437c4a rpc: recommend -rpcclienttimeout=0 for waitfor* (Sjors Provoost)
77ec072925a6d558b3c6b075becbed44727c5989 rpc: fix waitfornewblock description (Sjors Provoost)
285fe9fb51c808a9edd91b05bd3134fc18de0fb6 rpc: add test for waitforblock and waitfornewblock (Sjors Provoost)
b94b27cf05c709674117e308e441a8d1efddcd0a Add waitTipChanged to Mining interface (Sjors Provoost)
7eccdaf16081d6f624c4dc21df75b0474e049d2b node: Track last block that received a blockTip notification (Sjors Provoost)
ebb8215f23644f901c46fd4977b7d4b08fae5104 Rename getTipHash() to getTip() and return BlockRef (Sjors Provoost)
89a8f74bbbb900abfb3d8e946eea18ad7b1513ad refactor: rename BlockKey to BlockRef (Sjors Provoost)
Pull request description:
This continues the work in #30200 so that a future Stratum v2 Template Provider (see #29432) can avoid accessing node internals. It needs to know when a new block arrives in order to push new templates to connected clients.
`waitTipChanged()` uses a new kernel notification `notifications().m_tip_block_mutex`, which this PR also introduces (a previous version used `g_best_block`).
In order to ensure the new method works as intended, the `waitfornewblock`, `waitforblock` and `waitforblockheight` RPC methods are refactored to use it. This allows removing `RPCNotifyBlockChange`.
There's a commit to add (direct) tests for the methods that are about to be refactored:
- `waitfornewblock` was already implicitly tested by `feature_shutdown.py`.
- `waitforblockheight` by `feature_coinstatsindex.py` and `example_test.py`
This PR renames `getTipHash()` to `getTip()` and returns a `BlockRef` (renamed from `BlockKey`) so that callers can use either the height or hash.
The later commits make trivial improvements to the `waitfor*` RPC calls (not needed for this PR).
The `waitTipChanged()` method could probably also be used for the longpoll functionality in `getblocktemplate`, but I'm a bit reluctant to touch that.
`RPCServer::OnStarted` no longer does anything and `RPCServer::OnStopped` merely prints a log statement. They were added in #5711 as a refactor. This PR drops them entirely.
Finally `g_best_block` is also dropped.
ACKs for top commit:
achow101:
ACK 7942951e3fcc58f7db0059546d03be9ea243f1db
ryanofsky:
Code review ACK 7942951e3fcc58f7db0059546d03be9ea243f1db. Just rebased since last review
TheCharlatan:
Re-ACK 7942951e3fcc58f7db0059546d03be9ea243f1db
Tree-SHA512: a5559446b4000c95e07aad33284b7ee2e57aafd87e1ae778b3825d59689566d047a8047e47a10f76e6e341e7dc72fd265a65afbc0a9c011d17c4cafd55031837
5e190cd11f6ed8b7ab4db0f01192de63deaf6fd7 Replace CScript _hex_v_u8 appends with _hex (Lőrinc)
cac846c2fbf6fc69bfc288fd387aa3f68d84d584 Allow CScript's operator<< to accept spans, not just vectors (Lőrinc)
c78d8ff4cb83506413bb73833fc5c04885d0ece8 prevector: avoid GCC bogus warnings in insert method (Lőrinc)
Pull request description:
Split out of https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1722326803.
Replace `_hex_v_u8` for `CScript` appends to `_hex`, to skip vector conversion before serializing to the `prevector` in `CScript`.
To enable both `unsigned char` and `std::byte` values, I've extracted the existing serialization to append the size & data in separate private methods to clarify that it does more than just a simple data insertion.
There were also discussion on eliminating the operators here completely to obviate when we're serializing fixed-size collections as raw bytes, and when we're prefixing them with their size - should also be done in a separate PR.
ACKs for top commit:
achow101:
ACK 5e190cd11f6ed8b7ab4db0f01192de63deaf6fd7
ryanofsky:
Code review ACK 5e190cd11f6ed8b7ab4db0f01192de63deaf6fd7. Looks good!
hodlinator:
re-ACK 5e190cd11f6ed8b7ab4db0f01192de63deaf6fd7
Tree-SHA512: 27a646629e017b2a05416d5eb964dda8b25b900d466221eff7bfa1339ded443e1c5c4cf8ff20cb3bba915a2603787a9fa6f6ec12bc0b9415d9eb07b57289192b
284bd17309ab3b124d9dcddfec62f5506383343b add check that chainwork doesn't exceed minimum work (marcofleon)
9aa5d1c3fcd10ecb94310ad515a8569bc2d418f8 add clarification in comment (marcofleon)
Pull request description:
A followup to https://github.com/bitcoin/bitcoin/pull/30661
The added assertion just makes sure that the fuzz test is working as intended. If we're sure that the total work of the test chain is never more than minimum chain work, then we can be sure that the later assertion failure would actually mean that a bug in the headers presync logic was found.
This PR also addresses:
https://github.com/bitcoin/bitcoin/pull/30661#discussion_r1746614616https://github.com/bitcoin/bitcoin/pull/30661#discussion_r1764943665https://github.com/bitcoin/bitcoin/pull/30661#discussion_r1764961991
ACKs for top commit:
instagibbs:
reACK 284bd17309ab3b124d9dcddfec62f5506383343b
maflcko:
review ACK 284bd17309ab3b124d9dcddfec62f5506383343b
achow101:
ACK 284bd17309ab3b124d9dcddfec62f5506383343b
Tree-SHA512: 76a9dffea4b6e13499c636d6ad26af06135319d25117c0eb40cf8dfcfdca6a4549c9b4d2ba835192ca355e0f8d476227aeabf8bdb68770def72a9fb521533fe5
f482d0e366a84008129913b442f0c955de79ac93 fuzz: reduce number of iterations in `crypto_aeadchacha20poly1305` target (brunoerg)
Pull request description:
By reducing the number of iterations we improve the performance of this target and may increase coverage.
Running with `-runs=100000` from qa-assets I noticed a significant performance improvement and an increase on cov:
master:
```
#100000 DONE cov: 567 ft: 4078 corp: 124/33Kb lim: 4096 exec/s: 793 rss: 499Mb
```
PR:
```
#100000 DONE cov: 568 ft: 3833 corp: 113/15188b lim: 1746 exec/s: 1250 rss: 544Mb
```
ACKs for top commit:
achow101:
ACK f482d0e366a84008129913b442f0c955de79ac93
marcofleon:
Tested ACK f482d0e366a84008129913b442f0c955de79ac93. Saw the same slight increase in coverage. Executed 100,000 runs several times and total time went from 30-35 sec to 20-25 sec.
stratospher:
ACK f482d0e. saw similar coverage stats
Tree-SHA512: 1a96dbc22a0aed396b7f8cc9b13534b7f20a461f64f167c69c650529d535e360499f1a501abc1f957f7541ee1860b36a5580aa488a1edbfa0270c9ed83ef741d
51f7668d31e2624e41c7ce77fe33162802808f3f addrman: change nid_type from int to int64_t (Martin Zumsande)
051ba3290e30e210bfc50dea974063053313ad3e addrman, refactor: introduce user-defined type for internal nId (Martin Zumsande)
Pull request description:
With `nIdCount` being incremented for each addr received, an attacker could cause an overflow in the past, see https://bitcoincore.org/en/2024/07/31/disclose-addrman-int-overflow/
Even though that attack was made infeasible indirectly by addr rate-limiting (PR #22387), to be on the safe side and prevent any regressions change the `nId`s used internally to `int64_t`.
This is being done by first introducing a user-defined type for `nId`s in the first commit, and then updating it to `int64_t` (thanks sipa for help with this!).
Note that `nId` is only used internally, it is not part of the serialization, so `peers.dat` should not be affected by this.
I assume that the only reason this was not done in the past is to not draw attention to this previously undisclosed issue.
ACKs for top commit:
naumenkogs:
ACK 51f7668d31e2624e41c7ce77fe33162802808f3f
stratospher:
ACK 51f7668d31e2624e41c7ce77fe33162802808f3f. I think it's a good change to make the nId space large(64 bits) so that the nId values are distinct.
achow101:
ACK 51f7668d31e2624e41c7ce77fe33162802808f3f
Tree-SHA512: 68d4b8b0269a01a9544bedfa7c1348ffde00a288537e4c8bf2b88372ac7d96c4566a44dd6b06285f2fcf31b4f9336761e3bca7253fbc20db5e0d04e887156224
58499b00d0ad1c83e433caa8fcc0e5d3fd3f2070 refactor: move `SignSignature` helpers to test utils (Sebastian Falbesoner)
Pull request description:
These helpers haven't been used in production code since segwit was merged more than eight years ago (see commit 605e8473, PR #8149), so it seems appropriate to move them to the test utils module. As suggested by instagibbs, see https://github.com/bitcoin/bitcoin/pull/30352#discussion_r1697515508.
ACKs for top commit:
instagibbs:
ACK 58499b00d0ad1c83e433caa8fcc0e5d3fd3f2070
pablomartin4btc:
ACK 58499b00d0ad1c83e433caa8fcc0e5d3fd3f2070
Tree-SHA512: a52d3b92b477246f2ceb57c3690d0229a492b65a15dae331faeae9d96e5907f7fe1176edc1530243e0f088586984fd7ba435a0a2d2f2531c04d076fdf3f4095f
facbcd4cef8890ae18976fb53b67ea56b3c04454 log: Use ConstevalFormatString (MarcoFalke)
fae9b60c4ffef38d9725f42f992b1f38765312a3 test: Use LogPrintStr to test m_log_sourcelocations (MarcoFalke)
fa39b1ca63874db8ef8bc16b87e2699e8e1b67be doc: move-only logging warning (MarcoFalke)
Pull request description:
This changes all logging (including the wallet logging) to produce a
`ConstevalFormatString` at compile time, so that the format string can be
validated at compile-time.
I tested with `clang` and found that the compiler will use less than 1% more of time and memory.
When an error is found, the compile-time error depends on the compiler, but it may look similar to:
```
src/util/string.h: In function ‘int main(int, char**)’:
src/bitcoind.cpp:265:5: in ‘constexpr’ expansion of ‘util::ConstevalFormatString<1>(((const char*)"Hi %s %s"))’
src/util/string.h:38:98: in ‘constexpr’ expansion of ‘util::ConstevalFormatString<1>::Detail_CheckNumFormatSpecifiers(std::basic_string_view<char>(((const char*)((util::ConstevalFormatString<1>*)this)->util::ConstevalFormatString<1>::fmt)))’
src/util/string.h:78:34: error: expression ‘<throw-expression>’ is not a constant expression
78 | if (num_params != count) throw "Format specifier count must match the argument count!";
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
```
This refactor does not change behavior of the compiled executables.
ACKs for top commit:
hodlinator:
re-ACK facbcd4cef8890ae18976fb53b67ea56b3c04454
l0rinc:
ACK facbcd4cef8890ae18976fb53b67ea56b3c04454
ryanofsky:
Code review ACK facbcd4cef8890ae18976fb53b67ea56b3c04454
pablomartin4btc:
re-ACK facbcd4cef8890ae18976fb53b67ea56b3c04454
stickies-v:
Approach ACK and code LGTM facbcd4cef8890ae18976fb53b67ea56b3c04454 modulo a `tinyformat::format_error` concern.
Tree-SHA512: 852f74d360897020f0d0f6e5064edc5e7f7dacc2bec1d5feff22c634a2fcd2eb535aa75be0b7191d9053728be6108484c737154b02d68ad3186a2e5544ba0db8
a240e150e837b5a95ed19765a2e8b7c5b6013f35 streams: remove AutoFile::Get() entirely (Pieter Wuille)
e624a9bef16b6335fd119c10698352b59bf2930a streams: cache file position within AutoFile (Pieter Wuille)
Pull request description:
Fixes#30833.
Instead of relying on frequent `ftell` calls (which appear to cause a significant slowdown on some systems) in XOR-enabled `AutoFile`s, cache the file position within `AutoFile` itself.
ACKs for top commit:
achow101:
ACK a240e150e837b5a95ed19765a2e8b7c5b6013f35
davidgumberg:
untested reACK a240e150e8
theStack:
Code-review ACK a240e150e837b5a95ed19765a2e8b7c5b6013f35
Tree-SHA512: fd3681edc018afaf955dc7a41a0c953ca80d46c1129e3c5b306c87c95aae93b2fe7b900794eb8b6f10491f9211645e7939918a28838295e6873eb226fca7006f
e4e3b44e9cc7227b3ad765397c884999f57bac2e net: call `Select` with reachable networks in `ThreadOpenConnections` (brunoerg)
829becd990b504a2e8a57fa8a6ff6ac6ae8ff900 addrman: change `Select` to support multiple networks (brunoerg)
f698636ec86c004ab331994559c163b7319e6423 net: add `All()` in `ReachableNets` (brunoerg)
Pull request description:
This PR changes addrman's `Select` to support multiple networks and change `ThreadOpenConnections` to call it with reachable networks. It can avoid unnecessary `Select` calls and avoid exceeding the max number of tries (100), especially when turning a clearnet + Tor/I2P/CJDNS node to Tor/I2P/CJDNS. Compared to #29330, this approach is "less aggresive". It does not add a new init flag and does not impact address relay.
I did an experiment of calling `Select` without passing a network until it finds an address from a network that compose 20% ~ 25% of the addrman (limited to 100 tries).
ACKs for top commit:
achow101:
ACK e4e3b44e9cc7227b3ad765397c884999f57bac2e
vasild:
ACK e4e3b44e9cc7227b3ad765397c884999f57bac2e
naumenkogs:
ACK e4e3b44e9cc7227b3ad765397c884999f57bac2e
Tree-SHA512: e8466b72b85bbc2ad8bfb14471eb27d2c50d4e84218f5ede2c15a6fa3653af61b488cde492dbd398f7502bd847e95bfee1abb7e01092daba2236d3ce3d6d2268
a97f43d63a6e835bae20b0bc5d536df98f55d8a0 fuzz: Add harness for p2p headers sync (marcofleon)
a0eaa4749fe0f755e113eee70dee1989bdc07ad5 Add FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION in PoW check (marcofleon)
a3f6f5acd89f2f5bb136ec247f259d212e8944d0 build: Automatically define FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION for fuzz builds (marcofleon)
0c02d4b2bdbc7a3fc3031a63b3b16bafa669d51c net_processing: Make MAX_HEADERS_RESULTS a PeerManager option (marcofleon)
Pull request description:
This PR reopens https://github.com/bitcoin/bitcoin/pull/28043. It's a regression fuzz test for https://github.com/bitcoin/bitcoin/pull/26355 and [a couple bugs](ed6cddd98e) that were addressed in https://github.com/bitcoin/bitcoin/pull/25717. This should help us move forward with the [removal of mainnet checkpoints](https://github.com/bitcoin/bitcoin/pull/25725).
It seems like the main concern in https://github.com/bitcoin/bitcoin/pull/28043 was the global mock function for proof of work. This PR aims to be an improvement by replacing the previous approach with a fuzz build configured using `FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION`. This ensures that the simplified test code will never be in a release binary. If we agree this is the way to go, there are some other places (for future targets) where this method could be used.
In this target, PoW isn't being tested, so the goal is to bypass the check and let the fuzzer do its thing. In the other harnesses where PoW is actually being fuzzed, `CheckProofOfWork` is now `CheckProofOfWorkImpl`. So, the only change to that function is in the name.
More about `FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION` can be found at https://llvm.org/docs/LibFuzzer.html#fuzzer-friendly-build-mode and https://github.com/AFLplusplus/AFLplusplus/blob/stable/docs/fuzzing_in_depth.md#d-modifying-the-target.
ACKs for top commit:
naumenkogs:
ACK a97f43d63a6e835bae20b0bc5d536df98f55d8a0
dergoegge:
reACK a97f43d63a6e835bae20b0bc5d536df98f55d8a0
instagibbs:
tested ACK a97f43d63a6e835bae20b0bc5d536df98f55d8a0
brunoerg:
ACK a97f43d63a6e835bae20b0bc5d536df98f55d8a0
Tree-SHA512: 60b0bc6aadd8ca4c39db9cbba2da2debaaf68afcb6a8dd75c1ce48ca9e3996948fda8020930b6771a424e0f7c41b0b1068db4aa7dbe517f8fc152f1f712058ad
9ad2fe7e69e9e69949ebbb280a15756dc3301f09 clusterlin: only start/use search when enough iterations left (Pieter Wuille)
bd044356edb6c8978df600f33b0073f772ee047c clusterlin: improve heuristic to decide split transaction (optimization) (Pieter Wuille)
71f26293988019d2035bcc55af7b440b494b56bc clusterlin: include topological pot subsets automatically (optimization) (Pieter Wuille)
e20fda77a2da1da3deb273d83700a0b7139422ab clusterlin: reduce computation of unnecessary pot sets (optimization) (Pieter Wuille)
6060a948caf6dbc7505658d6cac750e25698eaf9 clusterlin bench: add example hard cluster benchmarks (Pieter Wuille)
2965fbf203f0b244814d7159381a2e9472bc1f97 clusterlin: track upper bound potential set for work items (optimization) (Pieter Wuille)
9e43e4ce109e98a1ea3f54bbb4de86bc1b92ae4f clusterlin: use feerate-sorted depgraph in SearchCandidateFinder (Pieter Wuille)
b80e6dfe780b3678bb41f2d9d63816f097529b2e clusterlin: add reordering support for DepGraph (Pieter Wuille)
85a285a306100d1815c4ad0f4e52ccbcae8b0fbc clusterlin: separate initial search entries per component (optimization) (Pieter Wuille)
e4faea9ca79ce1f277d8499962c7de64f26b0436 clusterlin bench: have low/high iter benchmarks instead of per-iter (Pieter Wuille)
Pull request description:
Part of cluster mempool: #30289
Depends on #30126, and was split off from it.
This improves the candidate search algorithm introduced in the previous PR with a variety of optimizations.
The resulting search algorithm largely follows Section 2 of [How to linearize your cluster](https://delvingbitcoin.org/t/how-to-linearize-your-cluster/303#h-2-finding-high-feerate-subsets-5), though with a few changes:
* Connected component analysis is performed inside the search algorithm (creating initial work items per component for each candidate), rather than once at a higher level. This duplicates some work but is significantly simpler in implementation.
* No ancestor-set based presplitting inside the search is performed; instead, the `best` value is initialized with the best topologically valid set known to the LIMO algorithm before search starts: the better one out of the highest-feerate remaining ancestor set, and the highest-feerate prefix of remaining transactions in `old_linearization`.
* Work items are represented using an included set *inc* and an undefined set *und*, rather than included and excluded.
* Potential sets *pot* are not computed for work items with empty *inc*.
At a high level, the only missing optimization from that post is bottleneck analysis; my thinking is that it only really helps with clusters that are already relatively cheap to linearize (doing so would need to be done at a higher level, not inside the search algorithm).
---
Overview of the impact of each commit here on linearize performance:
* **[clusterlin bench: have low/high iter benchmarks instead of per-iter](21a184db63)**: no impact
* **[separate initial search entries per component (optimization)](c84c5c86ba)**: reduce iterations, increase start-up cost
* **[add reordering support for DepGraph](019ff29609)**: no impact
* **[use feerate-sorted depgraph in SearchCandidateFinder](8e27dd5a22)**: typically reduce iterations, increase start-up cost
* **[track upper bound potential set for work items](781e0fb3aa)**: reduce iterations, increase cost per iteration
* **[reduce computation of unnecessary pot sets](9fe834fa97)**: reduce cost per iteration
* **[include topological pot subsets automatically](30612710a4)**: reduce iterations, increase cost per iteration
* **[improve heuristic to decide split transaction](1880c00ab1)**: typically reduce iterations, increase cost per iteration
* **[only start/use search when enough iterations left](12760a57b3)**: just account for start-up cost as equivalent iterations
ACKs for top commit:
sdaftuar:
ACK 9ad2fe7e69e9e69949ebbb280a15756dc3301f09
instagibbs:
reACK 9ad2fe7e69e9e69949ebbb280a15756dc3301f09
glozow:
reACK 9ad2fe7e69e, just have a question about the docs
Tree-SHA512: 108bcbb0676f36071eb83954059b5f3d6646c745015b644a2a5d7f5a8ac9424c2d01d339fa6318a3aff4cf313308e85bb80b0090899720a3fcba027b8025590a
282f0e92559da23e356504a564a0322b9888e50b Unit test runner documentation fix and improvements (Jon Atack)
Pull request description:
Running `test_bitcoin --help` prints the list of arguments that may be passed, not the list of tests, so fix that.
Improve the content and order of the unit test documentation.
ACKs for top commit:
pablomartin4btc:
re-ACK 282f0e92559da23e356504a564a0322b9888e50b
tdb3:
re ACK 282f0e92559da23e356504a564a0322b9888e50b
Tree-SHA512: 0d25108ab641bcd9b53f99d139afeec90a34f44d5b00c3c677f7539d87782440a28fadc348663b8c28ace43552834737b9c1e8f5517c68edc8547695a9cb5063
- Running `test_bitcoin --help` prints the list of arguments that may be passed,
not the list of tests, so fix that.
- Improve the content and order of the unit test documentation.
This test checks m_log_sourcelocations, not the formatting with format
specifiers. Those are tested in logging_LogPrintMacros below.
So just use LogPrintStr directly in this test, without format specifiers
and format args.
This is required for a follow-up commit.
Automatically add topologically-valid subsets of the potential set pot
to inc. It can be proven that these must be part of the best reachable
topologically-valid set from that work item.
This is a crucial optimization that (apparently) reduces the maximum
number of iterations from ~2^(N-1) to ~sqrt(2^N).
Co-Authored-By: Suhas Daftuar <sdaftuar@gmail.com>
In each work item, keep track of a conservative overestimate of the best
possible feerate that can be reached from it, and then use these to avoid
exploring hopeless work items.
Add a DepGraph(depgraph, reordering) function that constructs a new DepGraph
corresponding to an old one, but with its transactions is a modified order
(given as a vector from old to new positions).
Also use this reordering feature inside DepGraphFormatter::Unser, which needs
a small modification so that its reordering mapping is old-to-new (rather than
the new-to-old it used before).
Before this commit, the worst case for linearization involves clusters which
break apart in several smaller components after the first candidate is
included in the output linearization.
Address this by never considering work items that span multiple components
of what remains of the cluster.
fa5bc450d5d4c1d2daf7b205f1678402c3c21678 util: Use compile-time check for LogConnectFailure (MarcoFalke)
fa7087b896c0150c29d7a27c53e0533831a2bf3b util: Use compile-time check for FatalErrorf (MarcoFalke)
faa62c0112f2b7ab69c80a5178f5b79217bed0a6 util: Add ConstevalFormatString (MarcoFalke)
fae7b83eb58d22ed83878561603991131372cdd7 lint: Remove forbidden functions from lint-format-strings.py (MarcoFalke)
Pull request description:
The `test/lint/lint-format-strings.py` was designed to count the number of format specifiers and assert that they are equal to the number of parameters passed to the format function. The goal seems reasonable, but the implementation has many problems:
* It is written in Python, meaning that C++ code can not be parsed correctly. Currently it relies on brittle regex and string parsing.
* Apart from the parsing errors, there are also many logic errors. For example, `count_format_specifiers` allows a mix of positional specifiers and non-positional specifiers, which can lead to runtime format bugs. Also, `count_format_specifiers` silently skipped over "special" format specifiers, which are valid in tinyformat, which again can lead to runtime format bugs being undetected.
* The brittle logic has a history of breaking in pull requests that are otherwise fine. This causes the CI to fail and the pull request being blocked from progress until the bug in the linter is fixed, or the code is rewritten to work around the bug.
* It is only run in the CI, or when the developer invokes the script. It would be better if the developer got the error message at compile-time, directly when writing the code.
Fix all issues by using a `consteval` checked format string in `FatalErrorf` and `LogConnectFailure`.
This is the first step toward https://github.com/bitcoin/bitcoin/issues/30530 and a follow-up will apply the approach to the other places.
ACKs for top commit:
stickies-v:
re-ACK fa5bc450d5d4c1d2daf7b205f1678402c3c21678
l0rinc:
ACK fa5bc450d5d4c1d2daf7b205f1678402c3c21678
hodlinator:
ACK fa5bc450d5d4c1d2daf7b205f1678402c3c21678
ryanofsky:
Code review ACK fa5bc450d5d4c1d2daf7b205f1678402c3c21678
Tree-SHA512: d6189096b16083143687ed1b1559cf4f92f97dd87bc5d00673e44f4fb9fce7bb7b215cfdfc39b6e6a24f0b75a79a03ededce966639e554f7172e1fc22cf015ae
1eac96a503b6bac3eaf5d0eb3d23ffde3bfbf9aa Compare FromUserHex result against other hex validators and parsers (Lőrinc)
19947863e16425e6a119e7a4819867292b1235ee Use BOOST_CHECK_EQUAL for optional, arith_uint256, uint256, uint160 (Lőrinc)
743ac30e349e181c26a2d2af0bcb93b0835ce521 Add std::optional support to Boost's equality check (Lőrinc)
Pull request description:
Enhanced `FromUserHex` coverage by:
* Added `std::optional` support to `BOOST_CHECK_EQUAL`, allowing direct comparisons of `std::optional<T>` with other `T` expected values.
* Increased fuzz testing for hex parsing to validate against other hex validators and parsers.
----
* Use BOOST_CHECK_EQUAL for https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1706637780 arith_uint256, uint256, uint160
Example error before:
> unknown location:0: fatal error: in "validation_chainstatemanager_tests/chainstatemanager_args": std::bad_optional_access: bad_optional_access
test/validation_chainstatemanager_tests.cpp:781: last checkpoint
after:
> test/validation_chainstatemanager_tests.cpp:801: error: in "validation_chainstatemanager_tests/chainstatemanager_args": check set_opts({"-assumevalid=0"}).assumed_valid_block == uint256::ZERO has failed [std::nullopt != 0000000000000000000000000000000000000000000000000000000000000000]
ACKs for top commit:
stickies-v:
re-ACK 1eac96a503b6bac3eaf5d0eb3d23ffde3bfbf9aa
ryanofsky:
Code review ACK 1eac96a503b6bac3eaf5d0eb3d23ffde3bfbf9aa. Only changes since last review were auto type and fuzz test tweaks.
hodlinator:
ACK 1eac96a503b6bac3eaf5d0eb3d23ffde3bfbf9aa
Tree-SHA512: f1d2c65f0ee4e97830700be5b330189207b11ed0c89a8cebf0f97d43308402a6b3732e10130c79a0c044f7d2eeabfb5359990825aadf02c4ec19428dcd982b00
