de0675f9de5feae1f070840ad7218b1378fb880b refactor: Move `transaction_identifier.h` to primitives (marcofleon)
6f068f65de17951dc459bc8637e5de15b84ca445 Remove implicit uint256 conversion and comparison (marcofleon)
9c24cda72edb2085edfa75296d6b42fab34433d9 refactor: Convert remaining instances from uint256 to Txid (marcofleon)
d2ecd6815d89c9b089b55bc96fdf93b023be8dda policy, refactor: Convert uint256 to Txid (marcofleon)
f6c0d1d23128f742dfdda253752cba7db9bb0679 mempool, refactor: Convert uint256 to Txid (marcofleon)
aeb0f783305c923ee7667c46ca0ff7e1b96ed45c refactor: Convert `mini_miner` from uint256 to Txid (marcofleon)
326f24472487dc7f447839136db2ccf60833e9a2 refactor: Convert RPCs and `merkleblock` from uint256 to Txid (marcofleon)
49b3d3a92a7250e80c56ff8c351cf1670e32c1a2 Clean up `FindTxForGetData` (marcofleon)
Pull request description:
This is the final leg of the [type safety refactor](https://github.com/bitcoin/bitcoin/pull/32189).
All of these changes are straightforward `uint256` --> `Txid` along with any necessary explicit conversions. Also, `transaction_identifier.h` is moved to primitives in the last commit, as `Txid` and `Wtxid` become fundamental types after this PR.
ACKs for top commit:
stickies-v:
re-ACK de0675f9de5feae1f070840ad7218b1378fb880b, no changes since a20724d926d5844168c6a13fa8293df8c8927efe except address review nits.
janb84:
re ACK de0675f9de5feae1f070840ad7218b1378fb880b
dergoegge:
re-ACK de0675f9de5feae1f070840ad7218b1378fb880b
theStack:
Code-review ACK de0675f9de5feae1f070840ad7218b1378fb880b
Tree-SHA512: 2413160fca7ab146a8d79d18ce3afcf7384cacc73c513d41928904aa453b4dd7a350064cee71e9c5d015da5904c7c81ac17603e50a47441ebc5b0c653235dd08
These remaining miscellaneous changes were identified by commenting out
the `operator const uint256&` conversion and the `Compare(const uint256&)`
method from `transaction_identifier.h`.
Historically, the headers have been bumped some time after a file has
been touched. Do it now to avoid having to touch them again in the
future for that reason.
-BEGIN VERIFY SCRIPT-
sed -i --regexp-extended 's;( 20[0-2][0-9])(-20[0-2][0-9])? The Bitcoin Core developers;\1-present The Bitcoin Core developers;g' $( git show --pretty="" --name-only HEAD~0 )
-END VERIFY SCRIPT-
This can be reproduced according to the developer notes with something
like
( cd ./src/ && ../contrib/devtools/run-clang-tidy.py -p ../bld-cmake -fix -j $(nproc) )
Also, the header related changes were done manually.
fab1e02086ceebd7d96417b7a386fe61158bfda9 refactor: Pass verification_progress into block tip notifications (MarcoFalke)
fa76b378e4b218fb4853088328b9b488de18dcd2 rpc: Round verificationprogress to exactly 1 for a recent tip (MarcoFalke)
faf6304bdfdf228354b4072b72f4c0ef90fdaade test: Use mockable time in GuessVerificationProgress (MarcoFalke)
Pull request description:
Some users really seem to care about this. While it shouldn't matter much, the diff is so trivial that it is probably worth doing.
Fixes#31127
One could also consider to split the field into two dedicated ones (https://github.com/bitcoin/bitcoin/issues/28847#issuecomment-1807115357), but this is left for a more involved follow-up and may also be controversial.
ACKs for top commit:
achow101:
ACK fab1e02086ceebd7d96417b7a386fe61158bfda9
pinheadmz:
ACK fab1e02086ceebd7d96417b7a386fe61158bfda9
sipa:
utACK fab1e02086ceebd7d96417b7a386fe61158bfda9
Tree-SHA512: a3c24e3c446d38fbad9399c1e7f1ffa7904490a3a7d12623b44e583b435cc8b5f1ba83b84d29c7ffaf22028bc909c7cec07202b825480449c6419d2a190938f5
a58cb3b1c12c8cb75a87375c50f94c4605bb805d qa: sanity check mined block have their coinbase timelocked to height (Antoine Poinsot)
8f2078af6a55448c003b3f7f3021955fbb351caa miner: timelock coinbase transactions (Antoine Poinsot)
788aeebf343526760fa8f3ed969ac3713212a5b6 qa: use prev height as nLockTime for coinbase txs created in unit tests (Antoine Poinsot)
c76dbe9b8b6f03b761a0ef97e1b8cd133b934714 qa: timelock coinbase transactions created in fuzz targets (Antoine Poinsot)
9c94069d8b6cf67a24eb03c51230a4f2b2bf2d64 contrib: timelock coinbase transactions in signet miner (Antoine Poinsot)
a5f52cfcc400ad0adb41a78c65b8abb971e0d622 qa: timelock coinbase transactions created in functional tests (Antoine Poinsot)
Pull request description:
The Consensus Cleanup soft fork proposal includes enforcing that coinbase transactions set their
nLockTime field to the block height minus 1, as well as their nSequence such as to not disable the
timelock. If such a fork were to be activated by Bitcoin users, miners need to be ready to produce
compliant blocks at the risk of losing substantial amounts mining would-be invalid blocks. As miners
are unfamously slow to upgrade, it's good to make this change as early as possible.
Although Bitcoin Core's GBT implementation does not provide the `coinbasetxn` field, and mining
pool software crafts the coinbase on its own, updating the Bitcoin Core mining code is a first step
toward convincing pools to update their (often closed source) code. A possible followup is also to
introduce new fields to GBT. In addition, this first step also makes it possible to test future
Consensus Cleanup changes.
The commit making the change also updates a bunch of seemingly-unrelated tests. This is because those tests were asserting error messages based on the txid of transactions involved, and changing the coinbase transaction structure necessarily changes the txid of all tests' transactions.
ACKs for top commit:
Sjors:
Code review ACK a58cb3b1c12c8cb75a87375c50f94c4605bb805d
achow101:
ACK a58cb3b1c12c8cb75a87375c50f94c4605bb805d
TheCharlatan:
Re-ACK a58cb3b1c12c8cb75a87375c50f94c4605bb805d
Tree-SHA512: a2aae009a187eb760d34435f518a895ee76c6b02a667eb030ddf6bd584da6e8eae2737d974dbf81a928d60c07bcb4820f055adc067e18d8819640db0240bb513
e3014017bacff42d8d69f3061ce1ee621aaa450a test: add IsActiveAfter tests for versionbits (Anthony Towns)
60950f77c35e54e2884cfc14ab67623f3e325099 versionbits: docstrings for BIP9Info (Anthony Towns)
7565563bc7a5bb98ebf03a7d6881912a74d3f302 tests: refactor versionbits fuzz test (Anthony Towns)
2e4e9b9608c722aaf767638e9dba498d8dc3e772 tests: refactor versionbits unit test (Anthony Towns)
525c00f91bb27d0f2a1b2e5532aebec7fac97d3a versionbits: Expose VersionBitsConditionChecker via impl header (Anthony Towns)
e74a7049b477d1853191ded75fdf25024a6e233f versionbits: Expose StateName function (Anthony Towns)
d00d1ed52c8ee95eeed665d68d6715a694bd4c1f versionbits: Split out internal details into impl header (Anthony Towns)
37b9b67a39554465104c9cf1a74690f40019dbad versionbits: Simplify VersionBitsCache API (Anthony Towns)
1198e7d2fd665bf2bc49fd26773d4fd5fbc2b716 versionbits: Move BIP9 status logic for getblocktemplate to versionbits (Anthony Towns)
b1e967c3ec92738affb22d3b58483ebcdd8dfea2 versionbits: Move getdeploymentinfo logic to versionbits (Anthony Towns)
3bd32c20550e69688a4ff02409fb34b9a637b9c4 versionbits: Move WarningBits logic from validation to versionbits (Anthony Towns)
5da119e5d0e61f0b583f0fe21b9a00ee815a3e46 versionbits: Change BIP9Stats to uint32_t types (Anthony Towns)
a679040ec19ef17f3f03988a52207f1c03af701e consensus/params: Move version bits period/threshold to bip9 param (Anthony Towns)
e9d617095d4ce9525a4337d33624cac9d6b4abe6 versionbits: Remove params from AbstractThresholdConditionChecker (Anthony Towns)
9bc41f1b48b2e0cc6abf9714e860a29989d7809c versionbits: Use std::array instead of C-style arrays (Anthony Towns)
Pull request description:
Increases the encapsulation/modularity of the versionbits code, moving more of the logic into the versionbits module rather than having it scattered across validation and rpc code. Updates unit/fuzz tests to test the actual code used rather than just a close approximation of it.
ACKs for top commit:
achow101:
ACK e3014017bacff42d8d69f3061ce1ee621aaa450a
TheCharlatan:
Re-ACK e3014017bacff42d8d69f3061ce1ee621aaa450a
darosior:
ACK e3014017bacff42d8d69f3061ce1ee621aaa450a
Tree-SHA512: 2978db5038354b56fa1dd6aafd511099e9c16504d6a88daeac2ff2702c87bcf3e55a32e2f0a7697e3de76963b68b9d5ede7976ee007e45862fa306911194496d
The Consensus Cleanup soft fork proposal includes enforcing that coinbase transactions set their
locktime field to the block height, minus 1 (as well as their nSequence such as to not disable the
timelock). If such a fork were to be activated by Bitcoin users, miners need to be ready to produce
compliant blocks at the risk of losing substantial amounts mining would-be invalid blocks. As miners
are unfamously slow to upgrade, it's good to make this change as early as possible.
Although Bitcoin Core's GBT implementation does not provide the "coinbasetxn" field, and mining
pool software crafts the coinbase on its own, updating the Bitcoin Core mining code is a first step
toward convincing pools to update their (often closed source) code. A possible followup is also to
introduce new fields to GBT. In addition, this first step also makes it possible to test future
Consensus Cleanup changes.
The changes to the seemingly-unrelated RBF tests is because these tests assert an error message
which may vary depending on the txid of the transactions used in the test. This commit changes the
coinbase transaction structure and therefore impact the txid of transactions in all tests.
The change to the "Bad snapshot" error message in the assumeutxo functional test is because this
specific test case reads into the txid of the next transaction in the snapshot and asserts the error
message based it gets on deserializing this txid as a coin for the previous transaction. As this
commit changes this txid it impacts the deserialization error raised.
63b534f97e591d4e107fd5148909852eb2965d27 fuzz: sanity check hardcoded snapshot in utxo_snapshot target (Antoine Poinsot)
3b85eba83abe561078c91f5a5c49cf26c682c19b test util: split up ConnectBlock from MineBlock (Antoine Poinsot)
d1527f6b88656ff4aab3c671c6d9780ea2ae986e qa: correct off-by-one in utxo snapshot fuzz target (Antoine Poinsot)
Pull request description:
The assumeutxo data for the fuzz target could change and invalidate the hash silently, preventing the fuzz target from reaching some code paths. Fix this by introducing a unit test which would break if the snapshot data the fuzz target relies on were to change.
In implementing this i noticed the height used for coins in the fuzz target is actually off-by-one (as if the first block in the created chain was the genesis but it's block `1`), so fix that too.
ACKs for top commit:
mzumsande:
Code Review ACK 63b534f97e591d4e107fd5148909852eb2965d27
fjahr:
tACK 63b534f97e591d4e107fd5148909852eb2965d27
Tree-SHA512: 2399b6e74db9b78aab8efba67c57a405d2d7d880ae3b7d8518a1c96cc6266f61f5e77722cd999adeac5d3e03e73d84cf9ae7bdbcc0afae198cc87049dde4012f
This commit is done in preparation for the next commit. Here, the block
tree options are moved to the blockmanager options and the block tree is
instantiated through a helper method of the BlockManager, which is
removed again in the next commit.
Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>
Rather than having the rule change period/threshold be constant for all
potential deployments on a chain, have it be specific to the deployment
itself. This both matches history (BIP 9 specified a 2016 block period
and 1916 block threshold; BIP 91 specified a 336 block period and 269
block threshold; and BIP 341 specified a 2016 block period and 1815
block threshold), and allows the code to be simplified, as only the
BIP9Deployment structure is needed, not the full Consensus::Params
structure.
2a92702bafca5c78b270a9502a22cb9deac02cfc init: Use size_t consistently for cache sizes (TheCharlatan)
65cde3621dbb9ac7d210d4926e7601c4adf5f498 kernel: Move default cache constants to caches (TheCharlatan)
8826cae285490439dc1f19b25fa70b2b9e62dfe8 kernel: Move non-kernel db cache size constants (TheCharlatan)
e758b26b85da27ef44f3d2c924f3f08e8c1f4fdf kernel: Move kernel-specific cache size options to kernel (TheCharlatan)
d5e2c4a4097c799433cfc5367c61568fad2c784e fuzz: Add fuzz test for checked and saturating add and left shift (TheCharlatan)
c03a2795a8e044d17835bbf03de0c64dc7b41da8 util: Add integer left shift helpers (TheCharlatan)
8bd5f8a38ce903c05606841ebed1902398cb0b14 [refactor] init: Simplify coinsdb cache calculation (TheCharlatan)
5db7d4d3d28bd1269a09955b4695135c86c4827d doc: Correct docstring describing max block tree db cache (TheCharlatan)
Pull request description:
Carrying non-kernel related fields in the cache sizes for the indexes is confusing for kernel library users. The cache sizes are set currently with magic numbers in bitcoin-chainstate. The comments for the cache size calculations are not completely clear. The constants for the cache sizes are also currently in `txdb.h`, which is not an ideal place for holding all cache size related constants.
Solve these things by moving the kernel-specific cache size fields to their own struct and moving the constants to either the node or the kernel cache sizes.
This slightly changes the way the cache is allocated if (and only if) the txindex and/or blockfilterindex is used. Since they are now given precedence over the block tree db cache, this results in a bit less cache being allocated to the block tree db, coinsdb and coins caches. The effect is negligible though, i.e. cache sizes with default dbcache reported through the logs are:
master:
```
Cache configuration:
* Using 2.0 MiB for block index database
* Using 56.0 MiB for transaction index database
* Using 49.0 MiB for basic block filter index database
* Using 8.0 MiB for chain state database
* Using 335.0 MiB for in-memory UTXO set (plus up to 286.1 MiB of unused mempool space)
```
this PR:
```
Cache configuration:
* Using 2.0 MiB for block index database
* Using 56.2 MiB for transaction index database
* Using 49.2 MiB for basic block filter index database
* Using 8.0 MiB for chain state database
* Using 334.5 MiB for in-memory UTXO set (plus up to 286.1 MiB of unused mempool space)
```
---
This PR is part of the [libbitcoinkernel project](https://github.com/bitcoin/bitcoin/issues/27587).
ACKs for top commit:
stickies-v:
re-ACK 2a92702bafca5c78b270a9502a22cb9deac02cfc
ryanofsky:
Code review ACK 2a92702bafca5c78b270a9502a22cb9deac02cfc. Changes since last review are fixing size options to use size_t instead of int64_t again, simplifying CheckedLeftShift more, and making other minor suggested cleanups
hodlinator:
re-ACK 2a92702bafca5c78b270a9502a22cb9deac02cfc
Tree-SHA512: 98376eaa0660b1b8c096a5ce1f3e7c8c30e7cd6644de36856c2d3e573108cfc9473c93ebb3952b7881047b5ae6c85c5b096e6726f30f35be58b98eca07c8c785
This avoids having to rely on implicit casts when passing them to the
various functions allocating the caches.
This also ensures that if the requested amount of db_cache does not fit
in a size_t, it is clamped to the maximum value of a size_t.
Also take this opportunity to make the total amounts of cache in the
chainstate manager a size_t too.
They are not related to the txdb, so a better place for them is the
new kernel and node cache file. Re-use the default amount of kernel
cache for the default node cache.
Carrying non-kernel related fields in the cache sizes for the indexes is
confusing for kernel library users. The cache sizes also are set
currently with magic numbers in bitcoin-chainstate. The comments for the
cache size calculations are also not completely clear.
Solve these things by moving the kernel-specific cache size fields to
their own struct.
This slightly changes the way the cache is allocated if the txindex
and/or blockfilterindex is used. Since they are now given precedence
over the block tree db cache, this results in a bit less cache being
allocated to the block tree db, coinsdb and coins caches. The effect is
negligible though, i.e. cache sizes with default dbcache reported
through the logs are:
master:
Cache configuration:
* Using 2.0 MiB for block index database
* Using 56.0 MiB for transaction index database
* Using 49.0 MiB for basic block filter index database
* Using 8.0 MiB for chain state database
* Using 335.0 MiB for in-memory UTXO set (plus up to 286.1 MiB of unused mempool space)
this branch:
Cache configuration:
* Using 2.0 MiB for block index database
* Using 56.2 MiB for transaction index database
* Using 49.2 MiB for basic block filter index database
* Using 8.0 MiB for chain state database
* Using 334.5 MiB for in-memory UTXO set (plus up to 286.1 MiB of unused mempool space)
Also known as Ephemeral Dust.
We try to ensure that dust is spent in blocks by requiring:
- ephemeral dust tx is 0-fee
- ephemeral dust tx only has one dust output
- If the ephemeral dust transaction has a child,
the dust is spent by by that child.
0-fee requirement means there is no incentive to mine
a transaction which doesn't have a child bringing its
own fees for the transaction package.
c189eec848e3c31f438151d4d3422718a29df3a3 doc: release note for mempoolrullrbf removal (Greg Sanders)
d47297c6aaba44672fdd19d817d9b11d2dc90bb7 rpc: Mark fullrbf and bip125-replaceable as deprecated (Greg Sanders)
04a5dcee8ab56f2089ab08192b97b67bc15bc3ba docs: remove requirement to signal bip125 (Greg Sanders)
111a23d9b3615094fbfdf6cc8c996adc3db2782c Remove -mempoolfullrbf option (Greg Sanders)
Pull request description:
Given https://github.com/bitcoin/bitcoin/pull/30493 and the related discussion on network uptake it's probably not helpful to have an option for a feature that will not be respected by the network in any meaningful way.
Wallet changes can be done in another PR on its own cadence to account for possible fingerprinting, waiting for fullrbf logic to permeate the network, etc.
ACKs for top commit:
stickies-v:
re-ACK c189eec848e3c31f438151d4d3422718a29df3a3
achow101:
ACK c189eec848e3c31f438151d4d3422718a29df3a3
murchandamus:
ACK c189eec848e3c31f438151d4d3422718a29df3a3
rkrux:
reACK c189eec848e3c31f438151d4d3422718a29df3a3
Tree-SHA512: 9447f88f8f291c56c5bde70af0a91b0a4f5163aaaf173370fbfdaa3c3fd0b44120b14d3a1977f7ee10e27ffe9453f8a70dd38aad0ffb8c39cf145049d2550730
The added regtest option -test=bip94 is only used in the functional
test for BIP94.
This is done because the default regtest consensus rules
should aim to follow to mainnet, not testnet.
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
0dd16d7118f10ac291a45644769121cbdfd2352f build: Add a pkg-config file for libbitcoinkernel (TheCharlatan)
45be32f8384398ad8d590137d05a6373aa827b75 build: Produce a usable static kernel library (TheCharlatan)
Pull request description:
Since the move to cmake, the kernel static library that is installed after a cmake --install build is unusable. It lacks symbols for the internal libraries, besides those defined in the kernel library target.
Fix this by explicitly installing all the required internal static libraries. To make usage of these installed libraries easy, add a pkg-config file that can be used during linking.
This patch can be tested with:
```
cmake -B build -DBUILD_SHARED_LIBS=OFF -DBUILD_KERNEL_LIB=ON
cmake --build build
cmake --install build
g++ -std=c++20 -o test_chainstate src/bitcoin-chainstate.cpp -I/home/drgrid/bitcoin/src $(pkg-config --libs --static libbitcoinkernel)
```
Attempts to solve #30801
ACKs for top commit:
hebasto:
ACK 0dd16d7118f10ac291a45644769121cbdfd2352f.
fanquake:
ACK 0dd16d7118f10ac291a45644769121cbdfd2352f - this looks like a good place to start.
ryanofsky:
Code review ACK 0dd16d7118f10ac291a45644769121cbdfd2352f
Tree-SHA512: 92f7bc959584bdc595f4aa6d0ab133355481075fe8564224fd7ac122fd7bdd75f98cf26ef0a6a7d84fd552d2258ddca1b674eca91122469a58bacc5f0a0ec2ef
