This adds a first version of the overall linearization interface, which given
a DepGraph constructs a good linearization, by incrementally including good
candidate sets (found using AncestorCandidateFinder and SearchCandidateFinder).
This introduces a bespoke fuzzing-focused serialization format for DepGraphs,
and then tests that this format can represent any graph, roundtrips, and then
uses that to test the correctness of DepGraph itself.
This forms the basis for future fuzz tests that need to work with interesting
graphs.
This primarily adds the DepGraph class, which encapsulates precomputed
ancestor/descendant information for a given transaction cluster, with a
number of utility features (inspectors for set feerates, computing
reduced parents/children, adding transactions, adding dependencies), which
will become needed in future commits.
d1592d2eee guix: use gcc-12 to compile winpthreads (fanquake)
b23690e821 guix: use GCC 12.4.0 over 12.3.0 (fanquake)
8b41ede55e guix: consolidate back to GCC 12 toolchain for all HOSTS (fanquake)
Pull request description:
This PR contains 3 changes:
* Bump GCC in Guix from [12.3.0 to 12.4.0](https://gcc.gnu.org/gcc-12/). A patch was sent upstream, https://lists.gnu.org/archive/html/guix-patches/2024-06/msg01025.html, but has not landed.
* Consolidate all build environments back to using a GCC 12 toolchain. After #21778, the macOS environment is no-longer pinned to 11 (12 would otherwise cause issues building cctools). So, instead of requiring all builders to compile an additional GCC toolchain, use 12.
* Use GCC 12 to compile winpthreads. Currently, GCC 11 is used; which became apparent in https://github.com/bitcoin/bitcoin/pull/30452#issuecomment-2244715566.
ACKs for top commit:
TheCharlatan:
ACK d1592d2eee
hebasto:
ACK d1592d2eee.
Tree-SHA512: e3aa1fa3e69500c93180e07cb4684661247ec6bc45245f746538d81406ff1d8777131590307496dda3287a112b6633e4991168586ca4c2036fa3a57b1efa9c87
f46b220256 fuzz: Use BasicTestingSetup for coins_view target (TheCharlatan)
9e2a723d5d test: Add arguments for creating a slimmer setup (TheCharlatan)
Pull request description:
This adds arguments to some of the testing setup constructors for creating an environment without networking and a validation interface. This is useful for improving the performance of the utxo snapshot fuzz test, which constructs a new TestingSetup on each iteration.
Using this slimmed down `TestingSetup` in future might also make the tests a bit faster when run in aggregate.
ACKs for top commit:
maflcko:
review ACK f46b220256
dergoegge:
utACK f46b220256
Tree-SHA512: 9dc62512b127b781fc9e2d8ef2b5a9b06ebb927a8294b6d872001c553984a7eb1f348e0257b32435b34b5505b5d0323f73bdd572a673da272d3e1e8538ab49d6
fac0c3d4bf doc: Add release notes for two pull requests (MarcoFalke)
fa7b57e5f5 refactor: Replace ParseHashStr with FromHex (MarcoFalke)
fa90777245 rest: Reject truncated hex txid early in getutxos parsing (MarcoFalke)
fab6ddbee6 refactor: Expose FromHex in transaction_identifier (MarcoFalke)
fad2991ba0 refactor: Implement strict uint256::FromHex() (MarcoFalke)
fa103db2bb scripted-diff: Rename SetHex to SetHexDeprecated (MarcoFalke)
fafe4b8051 test: refactor: Replace SetHex with uint256 constructor directly (MarcoFalke)
Pull request description:
In `rest_getutxos` truncated txids such as `aa` or `ff` are accepted. This is brittle at best.
Fix it by rejecting any truncated (or overlarge) input.
----
Review note: This also starts a major refactor to rework hex parsing in Bitcoin Core, meaning that a few refactor commits are included as well. They are explained individually in the commit message and the work will be continued in the future.
ACKs for top commit:
stickies-v:
re-ACK fac0c3d4bf - only doc and test updates to address review comments, thanks!
hodlinator:
ACK fac0c3d4bf
Tree-SHA512: 473feb3fcf6118443435d1dd321006135b0b54689bfbbcb1697bb5811a449bef51f475c715de6911ff3c4ea3bdb75f601861ff93347bc4414d6b9e5298105dd7
faa3598772 ci: Add missing qttools5-dev install to Asan task (MarcoFalke)
Pull request description:
This is required, according to the docs:
```
$ git grep --line-number 'qtbase5-dev qttools5-dev qttools5-dev-tools' doc
doc/build-unix.md:84: sudo apt-get install qtbase5-dev qttools5-dev qttools5-dev-tools
```
Also, needed for cmake.
ACKs for top commit:
hebasto:
ACK faa3598772.
Tree-SHA512: c986908f757d70d958267c1e902b5d7d94589360db61ddf7b9b398cd635b2172e83510c0c77fd6032810166342a286c0f95225b6c6639acd869e1e51c3348ea7
25bf86a225 [test]: ensure `estimatesmartfee` default mode is `economical` (ismaelsadeeq)
41a2545046 [fees]: change `estimatesmartfee` default mode to `economical` (ismaelsadeeq)
Pull request description:
Fixes#30009
This PR changes the `estimatesmartfee` default mode to `economical`.
This was also suggested on IRC https://bitcoin-irc.chaincode.com/bitcoin-core-dev/2024-04-26#1021609
- `conservative` mode: This is the `estimatesmartfee` RPC mode which considers a longer history of blocks. It potentially returns a higher fee rate and is more likely to be sufficient for the desired target, but it is not as responsive to short-term drops in the prevailing fee market.
- `economical` mode: This is the `estimatesmartfee` RPC mode where estimates are potentially lower and more responsive to short-term drops in the prevailing fee market.
Since users are likely to use the default mode, this change will reduce overestimation for many users. The conservative mode remains available for those who wish to opt-in.
For an in-depth analysis of how significantly the `conservative` mode overestimates, see
https://delvingbitcoin.org/t/bitcoind-policy-estimator-modes-analysis/964.
ACKs for top commit:
instagibbs:
reACK 25bf86a225
glozow:
ACK 25bf86a225
willcl-ark:
ACK 25bf86a225
Tree-SHA512: 78ebda667eb9c8f87dcc2f0e6c14968bd1de30358dc77a13611b186fb8427ad97d9f537bad6e32e0a1aa477ccd8c64fee4d41e19308ef3cb184ff1664e6ba8a6
e3edaccd9d ci: add _LIBCPP_REMOVE_TRANSITIVE_INCLUDES to TSAN job (fanquake)
6e786165ca refactor: fix missing includes (fanquake)
Pull request description:
Add `_LIBCPP_REMOVE_TRANSITIVE_INCLUDES` to one of the libc++ CI jobs, to catch missing includes, that are otherwise hidden by transitive includes inside libc++. A more appropriate place for this might be the tidy job, but that does not use libc++.
See https://libcxx.llvm.org/DesignDocs/HeaderRemovalPolicy.html for more information.
ACKs for top commit:
maflcko:
re-ACK e3edaccd9d
Tree-SHA512: 3fb2e9bbbf4bb1570633d52939875ee674d934b645a4037a309643f84ab69edf0fb5b6cfcbd02fa7d92052a64fa63f31979a58fede23593c4df7c33a8cb2953a
This is a safe replacement of the previous SetHex, which now returns an
optional to indicate success or failure.
The code is similar to the ParseHashStr helper, which will be removed in
a later commit.
These cause compile failures with _LIBCPP_REMOVE_TRANSITIVE_INCLUDES.
i.e:
```bash
In file included from init.cpp:8:
./init.h:46:54: error: no template named 'atomic' in namespace 'std'
46 | bool AppInitBasicSetup(const ArgsManager& args, std::atomic<int>& exit_status);
| ~~~~~^
1 error generated.
```
See: https://libcxx.llvm.org/DesignDocs/HeaderRemovalPolicy.html.
Now that m_txrequest and m_recent_confirmed_transactions are guarded by
the same mutex, there is no benefit to processing them separately.
Instead, just loop through pblock->vtx once.
1bc9f64bee contrib: assume binary existence in sec/sym checks (fanquake)
51d8f435c9 contrib: simplify ELF test-security-check (fanquake)
1810e20677 contrib: simplify PE test-security-check (fanquake)
6c9746ff92 contrib: simplify MACHO test-security-check (fanquake)
Pull request description:
The current `test-security-check` script is hard to understand, and change (i.e https://github.com/bitcoin/bitcoin/pull/29987/files#diff-52aa0cda44721f089e53b128cb1232a876006ef257b211655456b17dfb2ec712); tests are also not done in isolation (when-possible). Fix that, and add missing checks. Simplifies future toolchain/security/hardening changes.
ACKs for top commit:
hebasto:
ACK 1bc9f64bee (assuming my Guix hashes match; I'll provide them shortly).
TheCharlatan:
ACK 1bc9f64bee
Tree-SHA512: 1885d0ce63a94ffa61345327f919da20b63de6dd4148d6db3ee8bad4485253a36e8ab0dbee48cecc02ea35d139edfed75453af45fc364bcbef6fe16b6823bc7a
c85accecaf [refactor] delete EraseTxNoLock, just use EraseTx (glozow)
6ff84069a5 remove obsoleted TxOrphanage::m_mutex (glozow)
61745c7451 lock m_recent_confirmed_transactions using m_tx_download_mutex (glozow)
723ea0f9a5 remove obsoleted hashRecentRejectsChainTip (glozow)
18a4355250 update recent_rejects filters on ActiveTipChange (glozow)
36f170d879 add ValidationInterface::ActiveTipChange (glozow)
3eb1307df0 guard TxRequest and rejection caches with new mutex (glozow)
Pull request description:
See #27463 for full project tracking.
This contains the first few commits of #30110, which require some thinking about thread safety in review.
- Introduce a new `m_tx_download_mutex` which guards the transaction download data structures including `m_txrequest`, the rolling bloom filters, and `m_orphanage`. Later this should become the mutex guarding `TxDownloadManager`.
- `m_txrequest` doesn't need to be guarded using `cs_main` anymore
- `m_recent_confirmed_transactions` doesn't need its own lock anymore
- `m_orphanage` doesn't need its own lock anymore
- Adds a new `ValidationInterface` event, `ActiveTipChanged`, which is a synchronous callback whenever the tip of the active chainstate changes.
- Flush `m_recent_rejects` and `m_recent_rejects_reconsiderable` on `ActiveTipChanged` just once instead of checking the tip every time `AlreadyHaveTx` is called. This should speed up calls to that function (no longer comparing a block hash each time) and removes the need to lock `cs_main` every time it is called.
Motivation:
- These data structures need synchronization. While we are holding `m_tx_download_mutex`, these should hold:
- a tx hash in `m_txrequest` is not also in `m_orphanage`
- a tx hash in `m_txrequest` is not also in `m_recent_rejects` or `m_recent_confirmed_transactions`
- In the future, orphan resolution tracking should also be synchronized. If a tx has an entry in the orphan resolution tracker, it is also in `m_orphanage`, and not in `m_txrequest`, etc.
- Currently, `cs_main` is used to e.g. sync accesses to `m_txrequest`. We should not broaden the scope of things it locks.
- Currently, we need to know the current chainstate every time we call `AlreadyHaveTx` so we can decide whether we should update it. Every call compares the current tip hash with `hashRecentRejectsChainTip`. It is more efficient to have a validation interface callback that updates the rejection filters whenever the chain tip changes.
ACKs for top commit:
instagibbs:
reACK c85accecaf
dergoegge:
Code review ACK c85accecaf
theStack:
Light code-review ACK c85accecaf
hebasto:
ACK c85accecaf, I have reviewed the code and it looks OK.
Tree-SHA512: c3bd524b5de1cafc9a10770dadb484cc479d6d4c687d80dd0f176d339fd95f73b85cb44cb3b6b464d38a52e20feda00aa2a1da5a73339e31831687e4bd0aa0c5
a517029646 depends: switch to building expat with CMake (fanquake)
Pull request description:
Switch to building Expat with CMake, instead of Autotools.
ACKs for top commit:
hebasto:
re-ACK a517029646.
Tree-SHA512: ca040545dd83fb81a8b209aa24cae6e22eaeff04f44bdabc4454adf6ea63d34f4ae27bd5980c65db2d2542e23eb2712102719023c262ab63a933c90b5999c11e
SetHex is fragile, because it accepts any non-hex input or any length of
input, without error feedback. This can lead to issues when the input is
truncated or otherwise corrupted.
Document the problem by renaming the method.
In the future, the fragile method should be removed from the public
interface.
-BEGIN VERIFY SCRIPT-
sed -i 's/SetHex/SetHexDeprecated/g' $( git grep -l SetHex ./src )
-END VERIFY SCRIPT-
09ce3501fa fix: Make TxidFromString() respect string_view length (Hodlinator)
01e314ce0a refactor: Change base_blob::SetHex() to take std::string_view (Hodlinator)
2f5577dc2e test: uint256 - Garbage suffixes and zero padding (Hodlinator)
f11f816800 refactor: Make uint256_tests no longer use deprecated BOOST_CHECK() (Hodlinator)
f0eeee2dc1 test: Add test for TxidFromString() behavior (Ryan Ofsky)
Pull request description:
### Problem
Prior to this, `TxidFromString()` was passing `string_view::data()` into `uint256S()` which meant it would only receive the a naked `char*` pointer and potentially scan past the `string_view::length()` until it found a null terminator (or some other non-hex character).
Appears to have been a fully dormant bug as callers were either passing a string literal or `std::string` directly to `TxidFromFromString()`, meaning a null terminator always existed at `pointer[length()]`. Bug existed since original merge of `TxidFromString()`.
### Solution
Make `uint256S()` (and `base_blob::SetHex()`) take and operate on `std::string_view` instead of `const char*` and have `TxidFromString()` pass that in.
(PR was prompted by comment in https://github.com/bitcoin/bitcoin/pull/30377#issuecomment-2208857200 (referring to https://github.com/bitcoin/bitcoin/pull/28922#discussion_r1404437378)).
ACKs for top commit:
maflcko:
re-ACK 09ce3501fa🕓
paplorinc:
ACK 09ce3501fa
ryanofsky:
Code review ACK 09ce3501fa. I think the current code changes are about as small as you could make to fix the bug without introducing a string copy, and the surrounding test improvements are all very nice and welcome.
Tree-SHA512: c2c10551785fb6688d1e2492ba42a8eee4c19abbe8461bb0774d56a70c23cd6b0718d2641632890bee880c06202dee148126447dd2264eaed4f5fee7e1bcb581
29eafd5733 rpc: doc: use "output script" terminology consistently in "asm"/"hex" results (Sebastian Falbesoner)
Pull request description:
The wording "public key script" was likely chosen as a human-readable form of the technical term `scriptPubKey`, but it doesn't seem to be really widespread. Replace it by the more (probably most?) common term "output script" instead. Note that the argument for the `decodescript` RPC is not necessarily an output script (it could e.g. be also a redeem script), so in this case we just stay generic and use "script".
See also the draft BIP "Terminology for Transaction Components" (https://github.com/murchandamus/bips/blob/2022-04-tx-terminology/bip-tx-terminology.mediawiki) from murchandamus which suggests to use "output script" as well.
Affects the help text of the following RPCs:
- decodepsbt
- decoderawtransaction
- decodescript
- getblock (if verbosity=3)
- getrawtransaction (if verbosity=2,3)
- gettxout
ACKs for top commit:
maflcko:
ACK 29eafd5733
achow101:
ACK 29eafd5733
BrandonOdiwuor:
ACK 29eafd5733
tdb3:
ACK 29eafd5733
Tree-SHA512: 62eb92d42bc44e36dc3090df7b248a123868a74af253d2046de02086e688bf6ff98307b927ba2fee3d599f85e073aeb8eca90ed15105ca63b648b6796cfa340b
d63ef73800 test: Add loadtxoutset test with tip on snapshot block (Fabian Jahr)
c2f86d4bcb test: Remove already resolved assumeutxo todo comments (Fabian Jahr)
Pull request description:
The first commit removes three Todos that have been addressed previously (see commit message for details).
The second message resolves another todo by adding the missing test case. This is a special case of "the tip has more work than the snapshot" where the tip is the same block as the snapshot base block.
Related to #28648.
ACKs for top commit:
jrakibi:
ACK [d63ef73](d63ef73800)
achow101:
ACK d63ef73800
maflcko:
ACK d63ef73800
alfonsoromanz:
Re ACK d63ef73800
Tree-SHA512: 8d5a25fc0b26531db3a9740132694138f2103b7b42eeb1d4a64095bfc901c1372e23601c0855c7def84c8a4e185d10611e4e830c4e479f1b663ae6ed53abb130
Using GCC 11 for the macOS build hasn't been required since #21778, and
at this point, given a toolchain is still needed (#30206), it makes more
sense to (re-)use 12, rather than make all builders compile another
GCC toolchain.
Prior to this, passing string_view::data() into uint256S() meant the latter would only receive the a naked char* pointer and potentially scan past the string_view::length() until it found a null terminator (or some other non-hex character).
Appears to have been a fully dormant bug as callers were either passing a string literal or std::string directly to TxidFromFromString(), meaning null terminator always existed at pointer[length()]. Bug existed since original merge of TxidFromString(), discussed in https://github.com/bitcoin/bitcoin/pull/28922#discussion_r1404437378.
6a5e9e40e1 doc: use proper doxygen formatting for CTxMemPool::cs (Vasil Dimov)
Pull request description:
Having `@par title` followed by an empty line renders improperly in Doxygen - it results in a paragraph with a title but without a body.
https://www.doxygen.nl/manual/commands.html#cmdpar
This also results in a compiler warning (or error) with Clang 19:
```
./txmempool.h:368:34: error: empty paragraph passed to '@par' command [-Werror,-Wdocumentation]
368 | * @par Consistency guarantees
| ~~~~~~~~~~~~~~~~~~~~~~~~~~^
1 error generated.
```
ACKs for top commit:
maflcko:
review ACK 6a5e9e40e1
tdb3:
ACK 6a5e9e40e1
Tree-SHA512: 2c4c9e5fd4bd44754800a9bcfff74df101afc060b84451c45aa098e4ceb05a47f28a36f8473b31222552fad6339b752a148e6b1c7d41c2003f515b3eb4060902
