Move its ownership to the ChainstateManager class.
Next to simplifying usage of the kernel library by no longer requiring
manual setup of the cache prior to using validation code, it also slims
down the amount of memory allocated by BasicTestingSetup.
33c48c106c validation: Check if mempool exists before asserting in ActivateSnapshot (TheCharlatan)
Pull request description:
The mempool is an optional component of the chainstate manager, so don't assume its presence and instead check if it is there first.
ACKs for top commit:
maflcko:
re-ACK 33c48c106c
fjahr:
ACK 33c48c106c
Tree-SHA512: 7a3568d5b7af45efa7bf54bae7bac1f00dc99bc9d47a744d73594f283c952be9500168f680d72f4aee09761da4e878ddca83ba675cdea8ee9e44eeff00ac09da
ce8094246e random: replace construct/assign with explicit Reseed() (Pieter Wuille)
2ae392d561 random: use LogError for init failure (Pieter Wuille)
97e16f5704 tests: make fuzz tests (mostly) deterministic with fixed seed (Pieter Wuille)
2c91330dd6 random: cleanup order, comments, static (Pieter Wuille)
8e31cf9c9b net, net_processing: use existing RNG objects more (Pieter Wuille)
d5fcbe966b random: improve precision of MakeExponentiallyDistributed (Pieter Wuille)
cfb0dfe2cf random: convert GetExponentialRand into rand_exp_duration (Pieter Wuille)
4eaa239dc3 random: convert GetRand{Micros,Millis} into randrange (Pieter Wuille)
82de1b80d9 net: use GetRandMicros for cache expiration (Pieter Wuille)
ddc184d999 random: get rid of GetRand by inlining (Pieter Wuille)
e2d1f84858 random: make GetRand() support entire range (incl. max) (Pieter Wuille)
810cdf6b4e tests: overhaul deterministic test randomness (Pieter Wuille)
6cfdc5b104 random: convert XoRoShiRo128PlusPlus into full RNG (Pieter Wuille)
8cc2f45065 random: move XoRoShiRo128PlusPlus into random module (Pieter Wuille)
8f5ac0d0b6 xoroshiro128plusplus: drop comment about nonexisting copy() (Pieter Wuille)
8924f5120f random: modernize XoRoShiRo128PlusPlus a bit (Pieter Wuille)
ddb7d26cfd random: add RandomMixin::randbits with compile-known bits (Pieter Wuille)
21ce9d8658 random: Improve RandomMixin::randbits (Pieter Wuille)
9b14d3d2da random: refactor: move rand* utilities to RandomMixin (Pieter Wuille)
40dd86fc3b random: use BasicByte concept in randbytes (Pieter Wuille)
27cefc7fd6 random: add a few noexcepts to FastRandomContext (Pieter Wuille)
b3b382dde2 random: move rand256() and randbytes() to .h file (Pieter Wuille)
493a2e024e random: write rand256() in function of fillrand() (Pieter Wuille)
Pull request description:
This PR contains a number of vaguely-related improvements to the random module.
The specific changes and more detailed rationale is in the commit messages, but the highlights are:
* `XoRoShiRo128PlusPlus` (previously a test-only RNG) moves to random.h and becomes `InsecureRandomContext`, which is even faster than `FastRandomContext` but non-cryptographic. It also gets all helper randomness functions (`randrange`, `fillrand`, ...), making it a lot more succinct to use.
* During tests, **all** randomness is made deterministic (except for `GetStrongRandBytes`) but non-repeating (like `GetRand()` used to be when `g_mock_deterministic_tests` was used), either fixed, or from a random seed (overridden by env var).
* Several infrequently used top-level functions (`GetRandMillis`, `GetRandMicros`, `GetExponentialRand`) are converted into member functions of `FastRandomContext` (and `InsecureRandomContext`).
* `GetRand<T>()` (without argument) can now return the maximum value of the type (previously e.g. `GetRand<uint32_t>()` would never return 0xffffffff).
ACKs for top commit:
achow101:
ACK ce8094246e
maflcko:
re-ACK ce8094246e🐈
hodlinator:
ACK ce8094246e
dergoegge:
utACK ce8094246e
Tree-SHA512: 79bc0cbafaf27e95012c1ce2947a8ca6f9a3c78af5f1f16e69354b6fc9b987a28858adf4cd356dc5baf21163e9af8dcc24e70f8d7173be870e8a3ddcdd47c02c
926b8e39dc [doc] add release note for TRUC (glozow)
19a9b90617 use version=3 instead of v3 in debug strings (glozow)
881fac8e60 scripted-diff: change names from V3 to TRUC (glozow)
a573dd2617 [doc] replace mentions of v3 with TRUC (glozow)
089b5757df rename mempool_accept_v3.py to mempool_truc.py (glozow)
f543852a89 rename policy/v3_policy.* to policy/truc_policy.* (glozow)
Pull request description:
Adds a release note for TRUC policy which will be live in v28.0.
For clarity, replaces mentions of "v3" with "TRUC" in most places. Suggested in
- https://github.com/bitcoin/bitcoin/pull/29496#discussion_r1629749583
- https://github.com/bitcoin/bitcoin/pull/29496#discussion_r1624500904
I changed error strings from "v3-violation" to "TRUC-violation" but left v3 in the debug strings because I think it might be clearer for somebody who is debugging. Similarly, I left some variables unchanged because I think they're more descriptive this way, e.g. `tx_v3_from_v2_and_v3`. I'm happy to debate places that should or shouldn't be documented differently in this PR, whatever is clearest to everyone.
ACKs for top commit:
instagibbs:
reACK 926b8e39dc
achow101:
ACK 926b8e39dc
ismaelsadeeq:
Code review ACK 926b8e39dc
Tree-SHA512: 16c88add0a29dc6d1236c4d45f34a17b850f6727b231953cbd52eb9f7268d1d802563eadfc8b7928c94ed3d7a615275dd103e57e81439ebf3ba2b12efa1e42af
2f9bde69f4 test: Remove unnecessary restart in assumeutxo test (Fabian Jahr)
19ce3d407e assumeutxo: Check snapshot base block is not marked invalid (Fabian Jahr)
80315c0118 refactor: Move early loadtxoutset checks into ActiveSnapshot (Fabian Jahr)
Pull request description:
This was discovered in a discussion in #29996
If the base block of the snapshot is marked invalid or part of an invalid chain, we currently still load the snapshot and get stuck in a weird state where we have the snapshot chainstate but it will never connect to our valid chain.
While this scenario is highly unlikely to occur on mainnet, it still seems good to prevent this inconsistent state.
The behavior change described above is in the second commit.
The first commit refactors the early checks in the `loadtxoutset` RPC by moving them into `ActivateSnapshot()` in order to have the chance to cover them by unit tests in the future and have a more consistent interface. Previously checks were spread out between `rpc/blockchain.cpp` and `validation.cpp`. In order to be able to return the error message to users of the RPC, the return type of `ActivateSnapshot()` is changed from `bool` to `util::Result`.
The third commit removes an unnecessary restart introduced in #29428.
ACKs for top commit:
mzumsande:
re-ACK 2f9bde6
alfonsoromanz:
Re-ACK 2f9bde69f4. The RPC code looks much cleaner after the refactor. Also, it seems very useful to get the error message in the RPC response rather than having to rely on the logs in some scenarios if you are an RPC user.
achow101:
ACK 2f9bde69f4
Tree-SHA512: 5328dd88c3c7be3f1be97c9eef52ac3666c27188c30a798b3e949f3ffcb83be075127c107e4046f7f39f961a79911ea3d61b61f3c11e451b3e4c541c264eeed4
Keep mentions of v3 in debug strings to help people who might not know
that TRUC is applied when version=3.
Also keep variable names in tests, as it is less verbose to keep v3 and v2.
94ed4fbf8e Add release note for size 2 package rbf (Greg Sanders)
afd52d8e63 doc: update package RBF comment (Greg Sanders)
6e3c4394cf mempool: Improve logging of replaced transactions (Greg Sanders)
d3466e4cc5 CheckPackageMempoolAcceptResult: Check package rbf invariants (Greg Sanders)
316d7b63c9 Fuzz: pass mempool to CheckPackageMempoolAcceptResult (Greg Sanders)
4d15bcf448 [test] package rbf (glozow)
dc21f61c72 [policy] package rbf (Suhas Daftuar)
5da3967815 PackageV3Checks: Relax assumptions (Greg Sanders)
Pull request description:
Allows any 2 transaction package with no in-mempool ancestors to do package RBF when directly conflicting with other mempool clusters of size two or less.
Proposed validation steps:
1) If the transaction package is of size 1, legacy rbf rules apply.
2) Otherwise the transaction package consists of a (parent, child) pair with no other in-mempool ancestors (or descendants, obviously), so it is also going to create a cluster of size 2. If larger, fail.
3) The package rbf may not evict more than 100 transactions from the mempool(bip125 rule 5)
4) The package is a single chunk
5) Every directly conflicted mempool transaction is connected to at most 1 other in-mempool transaction (ie the cluster size of the conflict is at most 2).
6) Diagram check: We ensure that the replacement is strictly superior, improving the mempool
7) The total fee of the package, minus the total fee of what is being evicted, is at least the minrelayfee * size of the package (equivalent to bip125 rule 3 and 4)
Post-cluster mempool this will likely be expanded to general package rbf, but this is what we can safely support today.
ACKs for top commit:
achow101:
ACK 94ed4fbf8e
glozow:
reACK 94ed4fbf8e via range-diff
ismaelsadeeq:
re-ACK 94ed4fbf8e
theStack:
Code-review ACK 94ed4fbf8e
murchandamus:
utACK 94ed4fbf8e
Tree-SHA512: 9bd383e695964f362f147482bbf73b1e77c4d792bda2e91d7f30d74b3540a09146a5528baf86854a113005581e8c75f04737302517b7d5124296bd7a151e3992
260f8da71a refactor: remove warnings globals (stickies-v)
9c4b0b7ce4 node: update uiInterface whenever warnings updated (stickies-v)
b071ad9770 introduce and use the generalized `node::Warnings` interface (stickies-v)
20e616f864 move-only: move warnings from common to node (stickies-v)
bed29c481a refactor: remove unnecessary AppendWarning helper function (stickies-v)
Pull request description:
This PR:
- moves warnings from common to the node library and into the node namespace (as suggested in https://github.com/bitcoin/bitcoin/pull/29845#discussion_r1570069541)
- generalizes the warnings interface to `Warnings::Set()` and `Warnings::Unset()` methods, instead of having a separate function and globals for each warning. As a result, this simplifies the `kernel::Notifications` interface.
- removes warnings.cpp from the kernel library
- removes warning globals
- adds testing for the warning logic
Behaviour change introduced:
- the `-alertnotify` command is executed for all `KernelNotifications::warningSet` calls, which now also covers the `LARGE_WORK_INVALID_CHAIN` warning
- the GUI is updated automatically whenever a warning is (un)set, covering some code paths where it previously wouldn't be, e.g. when `node::AbortNode()` is called, or for the `LARGE_WORK_INVALID_CHAIN` warning
Some discussion points:
- ~is `const std::string& id` the best way to refer to warnings? Enums are an obvious alternative, but since we need to define warnings across libraries, strings seem like a straightforward solution.~ _edit: updated approach to use `node::Warning` and `kernel::Warning` enums._
ACKs for top commit:
achow101:
ACK 260f8da71a
ryanofsky:
Code review ACK 260f8da71a. Only change since last review was rebasing
TheCharlatan:
Re-ACK 260f8da71a
Tree-SHA512: a3fcedaee0d3ad64e9c111aeb30665162f98e0e72acd6a70b76ff2ddf4f0a34da4f97ce353c322a1668ca6ee4d8a81cc6e6d170c5bbeb7a43cffdaf66646b588
fae3a1f006 log: use error level for critical log messages (MarcoFalke)
Pull request description:
This picks up the first commit from https://github.com/bitcoin/bitcoin/pull/29231, but extends it to also cover cases that were missed in it.
As per https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#logging, LogError should be used for severe problems that require the node to shut down.
ACKs for top commit:
stickies-v:
re-ACK fae3a1f006, I'm ~0 on the latest force push as `user_error` was already logged at the right level through `GetNotifications().fatalError(user_error);` so I'd be in favour of deduplicating/cleaning up this logging logic but can be done in follow-up.
kevkevinpal:
ACK [fae3a1f](fae3a1f006)
achow101:
ACK fae3a1f006
Tree-SHA512: 3f99fd25d5a204d570a42d8fb2b450439aad7685692f9594cc813d97253c4df172a6ff3cf818959bfcf25dfcf8ee9a9c9ccc6028fcfcecdb47591e18c77ef246
Support package RBF where the conflicting package would result
in a mempool cluster of size two, and each of its direct
conflicts are also part of an up-to-size-2 mempool cluster.
This restricted topology allows for exact calculation
of miner scores for each side of the equation, reducing
the surface area for new pins, or incentive-incompatible
replacements.
This allows wallets to create simple CPFP packages
that can fee bump other simple CPFP packages. This,
leveraged with other restrictions such as V3 transactions,
can create pin-resistant applications.
Future package RBF relaxations can be considered when appropriate.
Co-authored-by: glozow <gloriajzhao@gmail.com>
Co-authored-by: Greg Sanders <gsanders87@gmail.com>
Instead of having separate warning functions (and globals) for each
different warning that can be raised, encapsulate this logic into
a single class and allow to (un)set any number of warnings.
Introduces behaviour change:
- the `-alertnotify` command is executed for all
`KernelNotifications::warningSet` calls, which now also covers the
`LARGE_WORK_INVALID_CHAIN` warning.
- previously, warnings were returned based on a predetermined order,
e.g. with the "pre-release test build" warning always first. This
is no longer the case, and Warnings::GetMessages() will return
messages sorted by the id of the warning.
Removes warnings.cpp from kernel.
429ec1aaaa refactor: Rename CTransaction::nVersion to version (Ava Chow)
27e70f1f5b consensus: Store transaction nVersion as uint32_t (Ava Chow)
Pull request description:
Given that the use of a transaction's nVersion is always as an unsigned int, it doesn't make sense to store it as signed and then cast it to unsigned everywhere it is used and displayed.
Since a few alternative implementations have recently been revealed to have made an error with this signedness that would have resulted in consensus failure, I think it makes sense for us to just make this always unsigned to make it clear that the version is treated as unsigned. This would also help us avoid future potential issues with signedness of this value.
I believe that this is safe and does not actually change what transactions would or would not be considered both standard and consensus valid. Within consensus, the only use of the version in consensus is in BIP68 validation which was already casting it to uint32_t. Within policy, although it is used as a signed int for the transaction version number check, I do not think that this change would change standardness. Standard transactions are limited to the range [1, 2]. Negative numbers would have fallen under the < 1 condition, but by making it unsigned, they are still non-standard under the > 2 condition.
Unsigned and signed ints are serialized and unserialized the same way so there is no change in serialization.
ACKs for top commit:
maflcko:
ACK 429ec1aaaa 🐿
glozow:
ACK 429ec1aaaa
shaavan:
ACK 429ec1aaaa💯
Tree-SHA512: 0bcd92a245d7d16c3665d2d4e815a4ef28207ad4a1fb46c6f0203cdafeab1b82c4e95e4bdce7805d80a4f4a46074f6542abad708e970550d38a00d759e3dcef1
5bc2077e8f validation: allow to specify frequency for -checkblockindex (Martin Zumsande)
d5a631b959 validation: improve performance of CheckBlockIndex (Martin Zumsande)
32c80413fd bench: add benchmark for checkblockindex (Martin Zumsande)
Pull request description:
`CheckBlockIndex() ` are consistency checks that are currently enabled by default on regtest.
The function is rather slow, which is annoying if you
* attempt to run it on other networks, especially if not fully synced
* want to generate a long chain on regtest and see block generation slow down because you forgot to disable `-checkblockindex` or don't know it existed.
One reason why it's slow is that in order to be able to traverse the block tree depth-first from genesis, it inserts pointers to all block indices into a `std::multimap` - for which inserts and lookups become slow once there are hundred thousands of entries.
However, typically the block index is mostly chain-like with just a few forks so a multimap isn't really needed for the most part. This PR suggests to store the block indices of the chain ending in the best header in a vector instead, and store only the rest of the indices in a multimap. This does not change the actual consistency checks that are being performed for each index, just the way the block index tree is stored and traversed.
This adds a bit of complication to make sure each block is visited (note that there are asserts that check it), making sure that the two containers are traversed correctly, but it speeds up the function considerably:
On master, a single invocation of `CheckBlockIndex` takes ~1.4s on mainnet for me (4.9s on testnet which has >2.4 million blocks).
With this branch, the runtime goes down to ~0.27s (0.85s on testnet).This is a speedup by a factor ~5.
ACKs for top commit:
achow101:
ACK 5bc2077e8f
furszy:
ACK 5bc2077e8f
ryanofsky:
Code review ACK 5bc2077e8f. Just added suggested assert and simplification since last review
Tree-SHA512: 6b9c3e3e5069d6152b45a09040f962380d114851ff0f9ff1771cf8cad7bb4fa0ba25cd787ceaa3dfa5241fb249748e2ee6987af0ccb24b786a5301b2836f8487
f68cba29b3 blockman: Replace m_reindexing with m_blockfiles_indexed (Ryan Ofsky)
1b1c6dcca0 test: Add functional test for continuing a reindex (TheCharlatan)
201c1a9282 indexes: Don't wipe indexes again when already reindexing (TheCharlatan)
804f09dfa1 kernel: Add less confusing reindex options (Ryan Ofsky)
e172553223 validation: Remove needs_init from LoadBlockIndex (TheCharlatan)
533eab7d67 bugfix: Streamline setting reindex option (TheCharlatan)
Pull request description:
When restarting `bitcoind` during an ongoing reindex without setting the `-reindex` flag again, the block and coins db is left intact, but any data from the optional indexes is discarded. While not a bug per se, wiping the data again is
wasteful, both in terms of having to write it again, as well as potentially leading to longer startup times. So keep the index data instead when continuing a prior reindex.
Also includes a bugfix and smaller code cleanups around the reindexing code. The bug was introduced in b47bd95920: "kernel: De-globalize fReindex".
ACKs for top commit:
stickies-v:
ACK f68cba29b3
fjahr:
Code review ACK f68cba29b3
furszy:
Code review ACK f68cba29b3
ryanofsky:
Code review ACK f68cba29b3. Only changes since last review were cherry-picking suggested commits that rename variables, improving comments, and making some tweaks to test code.
Tree-SHA512: b252228cc76e9f1eaac56d5bd9e4eac23408e0fc04aeffd97a85417f046229364673ee1ca7410b9b6e7b692b03f13ece17c42a10176da0d7e975a8915deb98ca
As per doc/developer-notes#logging, LogError should be used for
severe problems that require the node to shut down.
Co-Authored-By: stickies-v <stickies-v@protonmail.com>
In order to ensure that the change of nVersion to a uint32_t in the
previous commit has no effect, rename nVersion to version in this commit
so that reviewers can easily spot if a spot was missed or if there is a
check somewhere whose semantics have changed.
This is a just a mechanical change, renaming and inverting the meaning
of the indexing variable.
"m_blockfiles_indexed" is a more straightforward name for this variable
because this variable just indicates whether or not
<datadir>/blocks/blk?????.dat files have been indexed in the
<datadir>/blocks/index LevelDB database. The name "m_reindexing" was
more confusing, it could be true even if -reindex was not specified, and
false when it was specified. Also, the previous name unnecessarily
required thinking about the whole reindexing process just to understand
simple checks in validation code about whether blocks were indexed.
The motivation for this change is to follow up on previous commits,
moving away from having multiple variables called "reindex" internally,
and instead naming variables individually after what they do and
represent.
30a01134cd [doc] update bips.md for 431 (glozow)
9dbe6a03f0 [test] wallet uses CURRENT_VERSION which is 2 (glozow)
539404fe0f [policy] make v3 transactions standard (glozow)
052ede75af [refactor] use TRUC_VERSION in place of 3 (glozow)
Pull request description:
Make `nVersion=3` (which is currently nonstandard on mainnet) standard.
Note that we will treat these transactions as Topologically Restricted Until Confirmation (TRUC). Spec is in BIP 431 and implementation is in #28948, #29306, and #29873
See #27463 for overall project tracking, and #29319 for information about relevance to cluster mempool.
ACKs for top commit:
sdaftuar:
utACK 30a01134c
achow101:
ACK 30a01134cd
instagibbs:
utACK 30a01134cd
murchandamus:
ACK 30a01134cd
ismaelsadeeq:
ACK 30a01134cd🛰️
Tree-SHA512: 2a4aec0442c860e792a061d83e36483c1f1b426f946efbdf664c8db97a596e498b535707e1d3a900218429486ea69fd4552e3d476526a6883cbd5556c6534b48
It does not control any actual logic and the log message as well as the
comment are obsolete, since no database initialization takes place there
anymore. Log messages indicating when indexes and chainstate databases
are loaded exist in other places.
2fd34ba504 Add sanity checks for various ATMPArgs booleans (Greg Sanders)
20d8936d8b [refactor] make some members MemPoolAccept-wide (glozow)
cbbfe719b2 cpfp carveout is excluded in packages (glozow)
69f7ab05ba Add m_allow_sibling_eviction as separate ATMPArgs flag (Greg Sanders)
57ee3029dd Add description for m_test_accept (Greg Sanders)
Pull request description:
First few commits of https://github.com/bitcoin/bitcoin/pull/28984 to set the stage for the package RBF logic.
These refactors are preparation for evaluating an RBF in a multi-proposed-transaction context instead of only a single proposed transaction. Also, carveouts and sibling evictions only should work in single RBF cases so add logic to preclude multi-tx cases in the future.
No behavior changes aside from bailing earlier from failed carve-outs.
ACKs for top commit:
glozow:
reACK 2fd34ba504 via range-diff
sr-gi:
utACK [2fd34ba](2fd34ba504)
theStack:
re-ACK 2fd34ba504
Tree-SHA512: 5071c5b8d9b8d2c9faa278c8c4df31de288cb407a68e4d55544c588caff6c86160cce7825453549c6ed69e29d9ccb5ee2d4a518b18f563bfb12f2ced073fe42a
542e13b293 rpc: Enhance metadata of the dumptxoutset output (Fabian Jahr)
4d8e5edbaa assumeutxo: Add documentation on dumptxoutset serialization format (Fabian Jahr)
c14ed7f384 assumeutxo: Add test for changed coin size value (Fabian Jahr)
de95953d87 rpc: Optimize serialization disk space of dumptxoutset (Fabian Jahr)
Pull request description:
The second attempt at implementing the `dumptxoutset` space optimization as suggested in #25675. Closes#25675.
This builds on the work done in #26045, addresses open feedback, adds some further improvements (most importantly usage of compact size), documentation, and an additional test.
The [original snapshot at height 830,000](https://github.com/bitcoin/bitcoin/pull/29551) came in at 10.82 GB. With this change, the same snapshot is 8.94 GB, a reduction of 17.4%.
This also enhances the metadata of the output file and adds the following data to allow for better error handling and make future upgrades easier:
- A newly introduced utxo set magic
- A version number
- The network magic
- The block height
ACKs for top commit:
achow101:
ACK 542e13b293
TheCharlatan:
Re-ACK 542e13b293
theStack:
ACK 542e13b293
Tree-SHA512: 0825d30e5c3c364062db3c6cbca4e3c680e6e6d3e259fa70c0c2b2a7020f24a47406a623582040988d5c7745b08649c31110df4c10656aa25f3f27eb35843d99
No change in behavior.
For single transaction acceptance, this is a simple refactor:
Workspace::m_all_conflicting
Workspace::m_conflicting_fees
Workspace::m_conflicting_size
Workspace::m_replaced_transactions
are now grouped under a new SubPackageState struct that is
a member of MemPoolAccept.
And local variables m_total_vsize and m_total_modified_fees are now
SubpackageState members so they can be accessed from
PackageMempoolChecks.
We want these to be package-wide variables because
- Transactions could conflict with the same tx (just not the same
prevout), or their conflicts could share descendants.
- We want to compare conflicts with the package fee rather than
individual transaction fee.
We reset these MemPoolAccept-wide fields for each subpackage
evaluation to not cause state leaking, similar to temporary
coins.
The behavior is not new, but this rule exits earlier than before.
Previously, a carve out could have been granted in PreChecks() but then
nullified in PackageMempoolChecks() when CheckPackageLimits() is called
with the default limits.
154b2b2296 [fuzz] V3_MAX_VSIZE and effective ancestor/descendant size limits (glozow)
a29f1df289 [policy] restrict all v3 transactions to 10kvB (glozow)
d578e2e354 [policy] explicitly require non-v3 for CPFP carve out (glozow)
Pull request description:
Opening for discussion / conceptual review.
We like the idea of a smaller maximum transaction size because:
- It lowers potential replacement cost (i.e. harder to do Rule 3 pinning via gigantic transaction)
- They are easier to bin-pack in block template production
- They equate to a tighter memory limit in data structures that are bounded by a number of transactions (e.g. orphanage and vExtraTxnForCompact). For example, the current memory bounds for orphanage is 100KvB * 100 = 40MB, and guaranteeing 1 tx per peer would require reserving a pretty large space.
History for `MAX_STANDARD_TX_WEIGHT=100KvB` (copied from https://github.com/bitcoin/bitcoin/pull/29873#issuecomment-2115459510):
- 2010-09-13 In 3df62878c3 satoshi added a 100kB (MAX_BLOCK_SIZE_GEN/5 with MBS_GEN = MAX_BLOCK_SIZE/2) limit on new transactions in CreateTransaction()
- 2013-02-04 https://github.com/bitcoin/bitcoin/pull/2273 In gavin gave that constant a name, and made it apply to transaction relay as well
Lowering `MAX_STANDARD_TX_WEIGHT` for all txns is not being proposed, as there are existing apps/protocols that rely on large transactions. However, it's been brought up that we should consider this for TRUCs (which is especially designed to avoid Rule 3 pinning).
This reduction should be ok because using nVersion=3 isn't standard yet, so this wouldn't break somebody's existing use case. If we find that this is too small, we can always increase it later. Decreasing would be much more difficult.
~[Expected size of a commitment transaction](https://github.com/lightning/bolts/blob/master/03-transactions.md#expected-weight-of-the-commitment-transaction) is within (900 + 172 * 483 + 224) / 4 = 21050vB~ EDIT: this is incorrect, but perhaps not something that should affect how we choose this number.
ACKs for top commit:
sdaftuar:
ACK 154b2b2296
achow101:
ACK 154b2b2296
instagibbs:
ACK 154b2b2296
t-bast:
ACK 154b2b2296
murchandamus:
crACK 154b2b2296
Tree-SHA512: 89392a460908a8ea9f547d90e00f5181de0eaa9d2c4f2766140a91294ade3229b3d181833cad9afc93a0d0e8c4b96ee2f5aeda7c50ad7e6f3a8320b9e0c5ae97
This carve out is intended to allow a second child under restricted
circumstances, but this topology is not allowed for v3 transactions.
As CPFP carve out does not explicitly require a second child to actually
exist, it has the effect of granting a free +10KvB descendant size limit
when a single child is enough to bust the descendant limit.
b47bd95920 kernel: De-globalize fReindex (TheCharlatan)
Pull request description:
fReindex is one of the last remaining globals exposed by the kernel library, so move it into the blockstorage class to reduce the amount of global mutable state and make the kernel library a bit less awkward to use.
---
This pull request is part of the [libbitcoinkernel project](https://github.com/bitcoin/bitcoin/issues/27587).
ACKs for top commit:
achow101:
ACK b47bd95920
ryanofsky:
Code review ACK b47bd95920. I rereviewed the whole PR, but the only change since last review was reverting the bugfix https://github.com/bitcoin/bitcoin/pull/29817#discussion_r1578327024 and make the change a pure refactoring.
mzumsande:
Code Review ACK b47bd95920
stickies-v:
ACK b47bd95920
Tree-SHA512: f7399d01f93bc0c0c7428fe95d19b9d29b4ed00a4f1deabca78fb0c4fecb434ec971e890feecb105938b5247c926850b1b7b4a4a9caa333a061e40777d0c8463
e41667b720 blockstorage: Don't move cursor backwards in UpdateBlockInfo (Ryan Ofsky)
17103637c6 blockstorage: Rename FindBlockPos and have it return a FlatFilePos (Martin Zumsande)
d9e477c4dc validation, blockstorage: Separate code paths for reindex and saving new blocks (Martin Zumsande)
064859bbad blockstorage: split up FindBlockPos function (Martin Zumsande)
fdae638e83 doc: Improve doc for functions involved in saving blocks to disk (Martin Zumsande)
0d114e3cb2 blockstorage: Add Assume for fKnown / snapshot chainstate (Martin Zumsande)
Pull request description:
`SaveBlockToDisk` / `FindBlockPos` are used for two purposes, depending on whether they are called during reindexing (`dbp` set, `fKnown = true`) or in the "normal" case when adding new blocks (`dbp == nullptr`, `fKnown = false`).
The actual tasks are quite different
- In normal mode, preparations for saving a new block are made, which is then saved: find the correct position on disk (maybe skipping to a new blk file), check for available disk space, update the blockfile info db, save the block.
- during reindex, most of this is not necessary (the block is already on disk after all), only the blockfile info needs to rebuilt because reindex wiped the leveldb it's saved in.
Using one function with many conditional statements for this leads to code that is hard to read / understand and bug-prone:
- many code paths in `FindBlockPos` are conditional on `fKnown` or `!fKnown`
- It's not really clear what actually needs to be done during reindex (we don't need to "save a block to disk" or "find a block pos" as the function names suggest)
- logic that should be applied to only one of the two modes is sometimes applied to both (see first commit, or #27039)
#24858 and #27039 were recent bugs directly related to the differences between reindexing and normal mode, and in both cases the simple fix took a long time to be reviewed and merged.
This PR proposes to clean this code up by splitting out the reindex logic into a separate function (`UpdateBlockInfo`) which will be called directly from validation. As a result, `SaveBlockToDisk` and `FindBlockPos` only need to cover the non-reindex logic.
ACKs for top commit:
paplorinc:
ACK e41667b720
TheCharlatan:
Re-ACK e41667b720
ryanofsky:
Code review ACK e41667b720. Just improvements to comments since last review.
Tree-SHA512: a14ff9a0facf6b1e3c1cd724a2d19a79a25d4b48de64398fdd172671532a472bc10a20cbb64ac3a3e55814dcc877d0597a3e1699cabc4f9d9a86b439b6eaba20
fReindex is one of the last remaining globals exposed by the kernel
library, so move it into the blockstorage class to reduce the amount of
global mutable state and make the kernel library a bit less awkward to
use.
cc67d33fda refactor: Simply include CTxMemPool::Options in CTxMemPool directly rather than duplicating definition (Luke Dashjr)
Pull request description:
Instead of duplicating mempool options two places, just include the Options struct directly on the CTxMemPool
ACKs for top commit:
achow101:
ACK cc67d33fda
kristapsk:
cr utACK cc67d33fda
jonatack:
ACK cc67d33fda
Tree-SHA512: 9deb5ea6f85eeb1c7e04536cded65303b0ec459936a97e4f257aff2c50b0984a4ddbf69a4651f48455b9c80200a1fd24e9c74926874fdd9be436bbbe406251ce
By calling SaveBlockToDisk only when we actually want to save a new
block to disk. In the reindex case, we now call UpdateBlockInfo
directly from validation.
This commit doesn't change behavior.
4a6d1d1e3b validation: don't clear cache on periodic flush (Andrew Toth)
Pull request description:
Since https://github.com/bitcoin/bitcoin/pull/17487 we no longer need to clear the coins cache when syncing to disk. A warm coins cache significantly speeds up block connection, and only needs to be fully flushed when nearing the `dbcache` limit.
Periodic flushes occur every 24 hours, which empties the cache and causes block connection to slow down. By keeping the cache through periodic flushes a node can run for several days with an increasingly hotter cache and connect blocks much more quickly. Now not only can setting a higher `dbcache` value be beneficial for IBD, it can also be beneficial for connecting blocks faster.
To benchmark in real world usage, I spun up 6 identical `t2.small` AWS EC2 instances, all running in the same region in the same VPC. I configured 2 instances to run master, 2 instances to run the change in this PR, and 2 instances to run the change in this PR but with `dbcache=1000`. All instances had `prune=5000` and a 20 GB `gp2` `EBS` volume. A 7th EC2 instance in the same VPC ran master and connected only to some trusted nodes in the outside network. Each of the 6 nodes under test only connected directly to this 7th instance. I manually pruned as much as possible and uploaded the same `blocks`, `chainstate` and `mempool.dat` to all instances. I started all 6 peers simultaneously at block height `835245` and ran them for over a week until block `836534`.
The results were much faster block connection times for this branch compared to master, and much faster for this branch with `dbcache=1000` compared to default `dbcache`.
| branch |speed |
|-----------:|----------:|
| master 1 | 1995.49ms/blk |
| master 2 | 2129.78ms/blk |
| branch default dbcache 1 | 1189.65ms/blk |
| branch default dbcache 2 | 1037.74ms/blk |
| branch dbcache=1000 1 | 393.69ms/blk |
| branch dbcache=1000 2 | 427.77ms/blk |
The log files of all 6 instances are [here](https://gist.github.com/andrewtoth/03c95033e7581d5dbc5be028639a1a91).
There is a lot of noise with the exact times of blocks being connected, so I plotted the rolling 20 block connect time averages. The large dots are the times where the cache is emptied. For the red master nodes, this happens every 24 hours. The blue branch nodes with default `dbcache` only filled up and emptied the caches once, which is seen in the middle. The green branch nodes with 1000 `dbcache` never emptied the cache. It is very clear from the chart that whenever the cache is emptied, connect block speed degrades significantly.
Also note that this still clears the cache for pruning flushes. Having frequent pruning flushes with a large cache that doesn't clear is less performant than the status quo https://github.com/bitcoin/bitcoin/pull/15265#issuecomment-458657451. See https://github.com/bitcoin/bitcoin/pull/28280.
ACKs for top commit:
sipa:
utACK 4a6d1d1e3b
achow101:
ACK 4a6d1d1e3b
brunoerg:
crACK 4a6d1d1e3b
Tree-SHA512: 05dbc677bc309bbcf89c52a6c5e853e2816b0ef0b5ee3719b30696df315a0427e244bb82da9ad828ec0e7ea8764552f8affe14c0184b52adf1909f5d8c1b4f9e
This makes it similar to -checkaddrman and -checkmempool, which
also allow to run the check occasionally instead of always / never.
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
by not saving all indexes in a std::multimap, but only
those that are not part of the best header chain.
The indexes of the best header chain are stored in a vector,
which, in the typical case of a mostly linear chain with
a few forks, results in a much smaller multimap, and increases
performance noticeably for long chains.
This does not change the actual consistency checks that are being
performed for each index, just the way the block index tree is
stored and traversed.
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
`util::Result` objects are aggregates that can hold multiple fields with
different information. Currently Result objects can only hold a success value
of an arbitrary type or a single bilingual_str error message. In followup PR
https://github.com/bitcoin/bitcoin/pull/25722, Result objects may be able to
hold both success and failure values of different types, plus error and warning
messages.
Having a Result::operator= assignment operator that completely erases all
existing Result information before assigning new information is potentially
dangerous in this case. For example, code that looks like it is assigning a
warning value could erase previously-assigned success or failure values.
Conversely, code that looks like it is just assigning a success or failure
value could erase previously assigned error and warning messages.
To prevent potential bugs like this, disable Result::operator= assignment
operator.
It is possible in the future we may want to re-enable operator= in limited
cases (such as when implicit conversions are not used) or add a Replace() or
Reset() method that mimicks default operator= behavior. Followup PR
https://github.com/bitcoin/bitcoin/pull/25722 also adds a Result::Update()
method providing another way to update an existing Result object.
Co-authored-by: stickies-v <stickies-v@protonmail.com>
b7ba60f81a test: add coverage for -reindex and assumeutxo (Martin Zumsande)
e57f951805 init, validation: Fix -reindex option with an existing snapshot (Martin Zumsande)
Pull request description:
In c711ca186f logic was introduced that `-reindex` and `-reindex-chainstate` will delete the snapshot chainstate.
This doesn't work currently, instead of deleting the snapshot chainstate the node crashes with an assert (this can be triggered by applying the added test commit on master).
Fix this, and another bug that would prevent the new active chainstate from having a mempool after `-reindex` has deleted the snapshot (also covered by the test).
ACKs for top commit:
fjahr:
re-ACK b7ba60f81a
hernanmarino:
crACK b7ba60f81a . Good fix
BrandonOdiwuor:
re-ACK b7ba60f81a
byaye:
Tested ACK b7ba60f81a
Tree-SHA512: c168f36997d7677d590af37b10427870f5d30123abf1c76032a16661e486735373bfa7e049e6aca439526fbcb6d619f970bf9d042196c851bf058a75a32fafdc
4ba1d0b553 fuzz: Add coverage for client_maxfeerate (Greg Sanders)
91d7d8f22a AcceptMultipleTransactions: Fix workspace client_maxfeerate (Greg Sanders)
f3aa5bd5eb fill_mempool: assertions and docsctring update (Greg Sanders)
a3da63e8fe Move fill_mempool to util function (Greg Sanders)
73b68bd8b4 fill_mempool: remove subtest-specific comment (Greg Sanders)
Pull request description:
Bug causes an `Assume()` failure due to the expectation that the individual result should be invalid when done over `submitpackage` via rpc.
Bug introduced by https://github.com/bitcoin/bitcoin/pull/28950 , and I discovered it rebasing https://github.com/bitcoin/bitcoin/pull/28984 since it's easier to hit in that test scenario.
Tests in place were only checking `AcceptSingleTransaction`-level checks due to package evaluation only triggering when minfee is too high for the parent transaction.
Added test along with fix, moving the fill_mempool utility into a common area for re-use.
ACKs for top commit:
glozow:
reACK 4ba1d0b553
theStack:
ACK 4ba1d0b553
ismaelsadeeq:
re-ACK 4ba1d0b553 via [diff](4fe7d150eb..4ba1d0b553)
Tree-SHA512: 3729bdf7f25d04e232f173ccee04ddbb2afdaafa3d04292a01cecf58fb11b3b2bc133e8490277f1a67622b62d17929c242dc980f9bb647896beea4332ee35306
This didn't work for two reasons:
1.) GetSnapshotCoinsDBPath() was used to retrieve the path.
This requires coins_views to exist, but the initialisation only happens later
(in CompleteChainstateInitialization) so the node hits an assert in
CCoinsViewDB& CoinsDB() and crashes.
2.) The snapshot was already activated, so it has the mempool attached.
Therefore, the mempool needs to be transferred back to the ibd
chainstate before deleting the snapshot chainstate.
824f47294a node: Use log levels in noui_ThreadSafeMessageBox (TheCharlatan)
ddc7872c08 node: Make translations of fatal errors consistent (TheCharlatan)
Pull request description:
The extra `bilingual_str` argument of the fatal error notifications and `node::AbortNode()` is often unused and when used usually contains the same string as the message argument. It also seems to be confusing, since it is not consistently used for errors requiring user action. For example some assumeutxo fatal errors require the user to do something, but are not translated.
So simplify the fatal error and abort node interfaces by only passing a translated string. This slightly changes the fatal errors displayed to the user.
ACKs for top commit:
stickies-v:
re-ACK 824f47294a
maflcko:
ACK 824f47294a🔎
achow101:
ACK 824f47294a
hebasto:
re-ACK 824f47294a.
Tree-SHA512: 2868ee7b045fe7f3ac582ce5039141b398480b7627734976201dafaaef7544b8461635a7292fee4a7f32ff1bfc26f9bd4d0c292dca424ba42fb7fc4483d7ce8d
fa4d98b3c8 Avoid divide-by-zero in header sync logs when NodeClock is behind (MarcoFalke)
fa58550317 refactor: Modernize header sync logs (MarcoFalke)
Pull request description:
The log may be confusing, when the NodeClock is behind the current header tip.
Fix it, by assuming the NodeClock is never behind the current header tip.
ACKs for top commit:
sipa:
utACK fa4d98b3c8
sr-gi:
tACK [fa4d98b](fa4d98b3c8)
achow101:
ACK fa4d98b3c8
tdb3:
ACK fa4d98b3c8
Tree-SHA512: 3c5aee4030af387695918c5238012c972ebf850b52e956b5f74590cd7fd4eff0b3e593d411e3eb2a0bb12294af8dc6fbe320f90e4c261399b65a404ff3c3cbd9
The extra `bilingual_str` argument of the fatal error notifications and
`node::AbortNode()` is often unused and when used usually contains the
same string as the message argument. It also seems to be confusing,
since it is not consistently used for errors requiring user action. For
example some assumeutxo fatal errors require the user to do something,
but are not translated.
So simplify the fatal error and abort node interfaces by only passing a
translated string. This slightly changes the fatal errors displayed to
the user.
Also de-duplicate the abort error log since it is repeated in noui.cpp.
9d9a7458a2 assumeutxo: Remove BLOCK_ASSUMED_VALID flag (Ryan Ofsky)
ef174e9ed2 test: assumeutxo snapshot block CheckBlockIndex crash test (Ryan Ofsky)
0391458d76 test: assumeutxo stale block CheckBlockIndex crash test (Ryan Ofsky)
ef29c8b662 assumeutxo: Get rid of faked nTx and nChainTx values (Ryan Ofsky)
9b97d5bbf9 doc: Improve comments describing setBlockIndexCandidates checks (Ryan Ofsky)
0fd915ee6b validation: Check GuessVerificationProgress is not called with disconnected block (Ryan Ofsky)
63e8fc912c ci: add getchaintxstats ubsan suppressions (Ryan Ofsky)
f252e687ec assumeutxo test: Add RPC test for fake nTx and nChainTx values (Ryan Ofsky)
Pull request description:
The `PopulateAndValidateSnapshot` function introduced in f6e2da5fb7 from #19806 has been setting fake `nTx` and `nChainTx` values that can show up in RPC results (https://github.com/bitcoin/bitcoin/issues/29328) and make `CBlockIndex` state hard to reason about, because it is difficult to know whether the values are real or fake.
Revert to previous behavior of setting `nTx` and `nChainTx` to 0 when the values are unknown, instead of faking them. Also drop no-longer needed `BLOCK_ASSUMED_VALID` flag.
Dropping the faked values also fixes assert failures in the `CheckBlockIndex` `(pindex->nChainTx == pindex->nTx + prev_chain_tx)` check that could happen previously if forked or out-of-order blocks before the snapshot got submitted while the snapshot was being validated. The PR includes two commits adding tests for these failures and describing them in detail.
Compatibility note: This change could cause new `-checkblockindex` failures if a snapshot was loaded by a previous version of Bitcoin Core and not fully validated, because fake `nTx` values will have been saved to the block index. It would be pretty easy to avoid these failures by adding some compatibility code to `LoadBlockIndex` and changing `nTx` values from 1 to 0 when they are fake (when `(pindex->nStatus & BLOCK_VALID_MASK) < BLOCK_VALID_TRANSACTIONS`), but a little simpler not to worry about being compatible in this case.
ACKs for top commit:
Sjors:
re-ACK 9d9a7458a2
achow101:
ACK 9d9a7458a2
mzumsande:
Tested ACK 9d9a7458a2
maflcko:
ACK 9d9a7458a2🎯
Tree-SHA512: b1e1e2731ec36be30d5f5914042517219378fc31486674030c29d9c7488ed83fb60ba7095600f469dc32f0d8ba79c49ff7706303006507654e1762f26ee416e0
38f70ba6ac RPC: Add maxfeerate and maxburnamount args to submitpackage (Greg Sanders)
Pull request description:
Resolves https://github.com/bitcoin/bitcoin/issues/28949
I couldn't manage to do it very cleanly outside of (sub)package evaluation itself, since it would change the current interface very heavily. Instead I threaded through the max fee argument and used that directly via ATMPArgs. From that perspective, this is somewhat a reversion from https://github.com/bitcoin/bitcoin/pull/19339. In a post-cluster mempool world, these checks could be consolidated to right after the given (ancestor) package is linearized/chunked, by just checking the feerate of the top chunk and rejecting the submission entirely if the top chunk is too high.
The implication here is that subpackages can be submitted to the mempool prior to hitting this new fee-based error condition.
ACKs for top commit:
ismaelsadeeq:
Re-ACK 38f70ba6ac👍🏾
glozow:
ACK 38f70ba6ac with some non-blocking nits
murchandamus:
LGTM, code review ACK 38f70ba6ac
Tree-SHA512: 38212aa9de25730944cee58b0806a3d37097e42719af8dd7de91ce86bb5d9770b6f7c37354bf418bd8ba571c52947da1dcdbb968bf429dd1dbdf8715315af18f
The `PopulateAndValidateSnapshot` function introduced in
f6e2da5fb7 from #19806 has been setting fake
`nTx` and `nChainTx` values that can show up in RPC results (see #29328) and
make `CBlockIndex` state hard to reason about, because it is difficult to know
whether the values are real or fake.
Revert to previous behavior of setting `nTx` and `nChainTx` to 0 when the
values are unknown, instead of faking them.
This commit fixes at least two assert failures in the (pindex->nChainTx ==
pindex->nTx + prev_chain_tx) check that would happen previously. Tests for
these failures are added separately in the next two commits.
Compatibility note: This change could result in -checkblockindex failures if a
snapshot was loaded by a previous version of Bitcoin Core and not fully
validated, because fake nTx values will have been saved to the block index. It
would be pretty easy to avoid these failures by adding some compatibility code
to `LoadBlockIndex` and changing `nTx` values from 1 to 0 when they are fake
(when `(pindex->nStatus & BLOCK_VALID_MASK) < BLOCK_VALID_TRANSACTIONS`), but a
little simpler not to worry about being compatible in this case.
And thread the feerate value through ProcessNewPackage to
reject individual transactions that exceed the given
feerate. This allows subpackage processing, and is
compatible with future package RBF work.
1342a31f3a [functional test] sibling eviction (glozow)
5fbab37859 [unit test] sibling not returned from SingleV3Checks if 1p2c or 3gen (glozow)
170306728a [policy] sibling eviction for v3 transactions (glozow)
b5d15f764f [refactor] return pair from SingleV3Checks (glozow)
Pull request description:
When we receive a v3 transaction that would bust a mempool transaction's descendant limit, instead of rejecting the new tx, consider replacing the other descendant if it is much higher feerate (using existing RBF criteria to assess that it's more incentive compatible and to avoid DoS).
Delving post with more background and motivation: https://delvingbitcoin.org/t/sibling-eviction-for-v3-transactions/472
ACKs for top commit:
sdaftuar:
ACK 1342a31f3a
achow101:
ACK 1342a31f3a
instagibbs:
ACK 1342a31f3a
Tree-SHA512: dd957d49e51db78758f566c49bddc579b72478e371275c592d3d5ba097d20de47a6c81952045021b99d82a787f5b799baf16dd0ee0e6de90ba12e21e275352be
This fixes the log output when -logsourcelocations is used.
Also, instead of 'ERROR:', the log will now say '[error]', like other
errors logged with LogError.
-BEGIN VERIFY SCRIPT-
sed -i --regexp-extended 's! error\("([^"]+)"! LogError("\1\\n"!g' $( git grep -l ' error(' ./src/ )
-END VERIFY SCRIPT-
This is needed for the next commit.
-BEGIN VERIFY SCRIPT-
# Separate sed invocations to replace one-line, and two-line error(...) calls
sed -i --regexp-extended 's!( +)return (error\(.*\);)!\1\2\n\1return false;!g' $( git grep -l 'return error(' )
sed -i --null-data --regexp-extended 's!( +)return (error\([^\n]*\n[^\n]*\);)!\1\2\n\1return false;!g' $( git grep -l 'return error(' )
-END VERIFY SCRIPT-
d5228efb53 kernel: Remove dependency on CScheduler (TheCharlatan)
06069b3913 scripted-diff: Rename MainSignals to ValidationSignals (TheCharlatan)
0d6d2b650d scripted-diff: Rename SingleThreadedSchedulerClient to SerialTaskRunner (TheCharlatan)
4abde2c4e3 [refactor] Make MainSignals RAII styled (TheCharlatan)
84f5c135b8 refactor: De-globalize g_signals (TheCharlatan)
473dd4b97a [refactor] Prepare for g_signals de-globalization (TheCharlatan)
3fba3d5dee [refactor] Make signals optional in mempool and chainman (TheCharlatan)
Pull request description:
By defining a virtual interface class for the scheduler client, users of the kernel can now define their own event consuming infrastructure, without having to spawn threads or rely on the scheduler design.
Removing `CScheduler` also allows removing the thread and exception modules from the kernel library.
To make the `CMainSignals` class easier to use from a kernel library perspective, remove its global instantiation and adopt RAII practices.
Renames `CMainSignals` to `ValidationSignals`, which more accurately describes its purpose and scope.
Also make the `ValidationSignals` in the `ChainstateManager` and CTxMemPool` optional. This could be useful in the future for using or testing these classes without having to instantiate any form of signal handling.
---
This PR is part of the [libbitcoinkernel project](https://github.com/bitcoin/bitcoin/issues/27587). It improves the kernel API and removes two modules from the kernel library.
ACKs for top commit:
maflcko:
re-ACK d5228efb53🌄
ryanofsky:
Code review ACK d5228efb53. Just comment change since last review.
vasild:
ACK d5228efb53
furszy:
diff ACK d5228ef
Tree-SHA512: e93a5f10eb6182effb84bb981859a7ce750e466efd8171045d8d9e7fe46e4065631d9f6f533c5967c4d34c9bb7d7a67e9f4593bd4c5b30cd7b3bbad7be7b331b
d8087adc7e [test] IsBlockMutated unit tests (dergoegge)
1ed2c98297 Add transaction_identifier::size to allow Span conversion (dergoegge)
1ec6bbeb8d [validation] Cache merkle root and witness commitment checks (dergoegge)
5bf4f5ba32 [test] Add regression test for #27608 (dergoegge)
49257c0304 [net processing] Don't process mutated blocks (dergoegge)
2d8495e080 [validation] Merkle root malleation should be caught by IsBlockMutated (dergoegge)
66abce1d98 [validation] Introduce IsBlockMutated (dergoegge)
e7669e1343 [refactor] Cleanup merkle root checks (dergoegge)
95bddb930a [validation] Isolate merkle root checks (dergoegge)
Pull request description:
This PR proposes to check for mutated blocks early as a defense-in-depth mitigation against attacks leveraging mutated blocks.
We introduce `IsBlockMutated` which catches all known forms of block malleation and use it to do an early mutation check whenever we receive a `block` message.
We have observed attacks that abused mutated blocks in the past, which could have been prevented by simply not processing mutated blocks (e.g. https://github.com/bitcoin/bitcoin/pull/27608 for which a regression test is included in this PR).
ACKs for top commit:
achow101:
ACK d8087adc7e
maflcko:
ACK d8087adc7e🏄
fjahr:
Code review ACK d8087adc7e
sr-gi:
Code review ACK d8087adc7e
Tree-SHA512: 618ff4ea7f168e10f07504d3651290efbb1bb2ab3b838ffff3527c028caf6c52dedad18d04d3dbc627977479710930e200f2dfae18a08f627efe7e64a57e535f
9d1dbbd4ce scripted-diff: Fix bitcoin_config_h includes (TheCharlatan)
Pull request description:
As mentioned in https://github.com/bitcoin/bitcoin/pull/26924#issuecomment-1403449932 and https://github.com/bitcoin/bitcoin/pull/29263#issuecomment-1922334399, it is currently not safe to remove `bitcoin-config.h` includes from headers because some unrelated file might be depending on it.
See also #26972 for discussion.
Solve this by including the file directly everywhere it's required, regardless of whether or not it's already included by another header.
There should be no functional change here, but it will allow us to safely remove includes from headers in the future.
~I'm afraid it's a bit tedious to reproduce these commits, but it's reasonably straightforward:~
Edit: See note below
```bash
# All commands executed from the src/ subdir.
# Collect all tokens from bitcoin-config.h.in
# Isolate the tokens and remove blank lines
# Replace newlines with | and remove the last trailing one
# Collect all files which use these tokens
# Filter out subprojects (proper forwarding can be verified from Makefiles)
# Filter out .rc files
# Save to a text file
git grep -E -l `grep undef config/bitcoin-config.h.in | cut -d" " -f2 | grep -v '^$' | tr '\n' '|' | sed 's/|$//'` | grep -v -e "^leveldb/" -e "^secp256k1/" -e "^crc32c/" -e "^minisketch/" -e "^Makefile" -e "\.rc$" > files-with-config-include.txt
# Find all files from the above list which don't include bitcoin-config.h
git grep -L -E "config/bitcoin-config.h" -- `cat files-with-config-include.txt`
# Include them manually with the exception of some files in crypto:
# crypto/sha256_arm_shani.cpp crypto/sha256_avx2.cpp crypto/sha256_sse41.cpp crypto/sha256_x86_shani.cpp
# These are exceptions which don't use bitcoin-config.h, rather the Makefile.am adds these cppflags manually.
# Commit changes. This should match the first commit of this PR.
# Use the same search as above to find all files which DON'T use any config tokens
git grep -E -L `grep undef config/bitcoin-config.h.in | cut -d" " -f2 | grep -v '^$' | tr '\n' '|' | sed 's/|$//'` | grep -v -e "^leveldb/" -e "^secp256k1/" -e "^crc32c/" -e "^minisketch/" -e "^Makefile" -e "\.rc$" > files-without-config-include.txt
# Manually remove the includes and commit changes. This should match the second commit of this PR.
```
Edit: I'll keep this old description for posterity, but the manual approach has been replaced with a scripted diff from TheCharlatan
ACKs for top commit:
maflcko:
ACK 9d1dbbd4ce🚪
TheCharlatan:
ACK 9d1dbbd4ce
hebasto:
ACK 9d1dbbd4ce, I have reviewed the code and it looks OK.
fanquake:
ACK 9d1dbbd4ce
Tree-SHA512: f11ddc4ae6a887f96b954a6b77f310558ddb271088a3fda3edc833669c4251b7f392515224bbb8e5f67eb2c799b4ffed3b07d96454e82ec635c686d0df545872
This is done in preparation for the next two commits, where the
CMainSignals are de-globalized.
This avoids adding new constructor arguments to the ChainstateManager
and CTxMemPool classes over the next two commits.
This could also allow future tests that are only interested in the
internal behaviour of the classes to forgo instantiating the signals.
-BEGIN VERIFY SCRIPT-
regex_string='^(?!//).*(AC_APPLE_UNIVERSAL_BUILD|BOOST_PROCESS_USE_STD_FS|CHAR_EQUALS_INT8|CLIENT_VERSION_BUILD|CLIENT_VERSION_IS_RELEASE|CLIENT_VERSION_MAJOR|CLIENT_VERSION_MINOR|COPYRIGHT_HOLDERS|COPYRIGHT_HOLDERS_FINAL|COPYRIGHT_HOLDERS_SUBSTITUTION|COPYRIGHT_YEAR|ENABLE_ARM_SHANI|ENABLE_AVX2|ENABLE_EXTERNAL_SIGNER|ENABLE_SSE41|ENABLE_TRACING|ENABLE_WALLET|ENABLE_X86_SHANI|ENABLE_ZMQ|HAVE_BOOST|HAVE_BUILTIN_CLZL|HAVE_BUILTIN_CLZLL|HAVE_BYTESWAP_H|HAVE_CLMUL|HAVE_CONSENSUS_LIB|HAVE_CXX20|HAVE_DECL_BE16TOH|HAVE_DECL_BE32TOH|HAVE_DECL_BE64TOH|HAVE_DECL_BSWAP_16|HAVE_DECL_BSWAP_32|HAVE_DECL_BSWAP_64|HAVE_DECL_FORK|HAVE_DECL_FREEIFADDRS|HAVE_DECL_GETIFADDRS|HAVE_DECL_HTOBE16|HAVE_DECL_HTOBE32|HAVE_DECL_HTOBE64|HAVE_DECL_HTOLE16|HAVE_DECL_HTOLE32|HAVE_DECL_HTOLE64|HAVE_DECL_LE16TOH|HAVE_DECL_LE32TOH|HAVE_DECL_LE64TOH|HAVE_DECL_PIPE2|HAVE_DECL_SETSID|HAVE_DECL_STRERROR_R|HAVE_DEFAULT_VISIBILITY_ATTRIBUTE|HAVE_DLFCN_H|HAVE_DLLEXPORT_ATTRIBUTE|HAVE_ENDIAN_H|HAVE_EVHTTP_CONNECTION_GET_PEER_CONST_CHAR|HAVE_FDATASYNC|HAVE_GETENTROPY_RAND|HAVE_GETRANDOM|HAVE_GMTIME_R|HAVE_INTTYPES_H|HAVE_LIBADVAPI32|HAVE_LIBCOMCTL32|HAVE_LIBCOMDLG32|HAVE_LIBGDI32|HAVE_LIBIPHLPAPI|HAVE_LIBKERNEL32|HAVE_LIBOLE32|HAVE_LIBOLEAUT32|HAVE_LIBSHELL32|HAVE_LIBSHLWAPI|HAVE_LIBUSER32|HAVE_LIBUUID|HAVE_LIBWINMM|HAVE_LIBWS2_32|HAVE_MALLOC_INFO|HAVE_MALLOPT_ARENA_MAX|HAVE_MINIUPNPC_MINIUPNPC_H|HAVE_MINIUPNPC_UPNPCOMMANDS_H|HAVE_MINIUPNPC_UPNPERRORS_H|HAVE_NATPMP_H|HAVE_O_CLOEXEC|HAVE_POSIX_FALLOCATE|HAVE_PTHREAD|HAVE_PTHREAD_PRIO_INHERIT|HAVE_STDINT_H|HAVE_STDIO_H|HAVE_STDLIB_H|HAVE_STRERROR_R|HAVE_STRINGS_H|HAVE_STRING_H|HAVE_STRONG_GETAUXVAL|HAVE_SYSCTL|HAVE_SYSCTL_ARND|HAVE_SYSTEM|HAVE_SYS_ENDIAN_H|HAVE_SYS_PRCTL_H|HAVE_SYS_RESOURCES_H|HAVE_SYS_SELECT_H|HAVE_SYS_STAT_H|HAVE_SYS_SYSCTL_H|HAVE_SYS_TYPES_H|HAVE_SYS_VMMETER_H|HAVE_THREAD_LOCAL|HAVE_TIMINGSAFE_BCMP|HAVE_UNISTD_H|HAVE_VM_VM_PARAM_H|LT_OBJDIR|PACKAGE_BUGREPORT|PACKAGE_NAME|PACKAGE_STRING|PACKAGE_TARNAME|PACKAGE_URL|PACKAGE_VERSION|PTHREAD_CREATE_JOINABLE|QT_QPA_PLATFORM_ANDROID|QT_QPA_PLATFORM_COCOA|QT_QPA_PLATFORM_MINIMAL|QT_QPA_PLATFORM_WINDOWS|QT_QPA_PLATFORM_XCB|QT_STATICPLUGIN|STDC_HEADERS|STRERROR_R_CHAR_P|USE_ASM|USE_BDB|USE_DBUS|USE_NATPMP|USE_QRCODE|USE_SQLITE|USE_UPNP|_FILE_OFFSET_BITS|_LARGE_FILES)'
exclusion_files=":(exclude)src/minisketch :(exclude)src/crc32c :(exclude)src/secp256k1 :(exclude)src/crypto/sha256_arm_shani.cpp :(exclude)src/crypto/sha256_avx2.cpp :(exclude)src/crypto/sha256_sse41.cpp :(exclude)src/crypto/sha256_x86_shani.cpp"
git grep --perl-regexp --files-with-matches "$regex_string" -- '*.cpp' $exclusion_files | xargs git grep -L "bitcoin-config.h" | while read -r file; do line_number=$(awk -v my_file="$file" '/\/\/ file COPYING or https?:\/\/www.opensource.org\/licenses\/mit-license.php\./ {line = NR} /^\/\// && NR == line + 1 {while(getline && /^\/\//) line = NR} END {print line+1}' "$file"); sed -i "${line_number}i\\\\n\#if defined(HAVE_CONFIG_H)\\n#include <config/bitcoin-config.h>\\n\#endif" "$file"; done;
git grep --perl-regexp --files-with-matches "$regex_string" -- '*.h' $exclusion_files | xargs git grep -L "bitcoin-config.h" | while read -r file; do sed -i "/#define.*_H/a \\\\n\#if defined(HAVE_CONFIG_H)\\n#include <config/bitcoin-config.h>\\n\#endif" "$file"; done;
for file in $(git grep --files-with-matches 'bitcoin-config.h' -- '*.cpp' '*.h' $exclusion_files); do if ! grep -q --perl-regexp "$regex_string" $file; then sed -i '/HAVE_CONFIG_H/{N;N;N;d;}' $file; fi; done;
-END VERIFY SCRIPT-
The first command creates a regular expression for matching all bitcoin-config.h symbols in the following form: ^(?!//).*(AC_APPLE_UNIVERSAL_BUILD|BOOST_PROCESS_USE_STD_FS|...|_LARGE_FILES). It was generated with:
./autogen.sh && printf '^(?!//).*(%s)' $(awk '/^#undef/ {print $2}' src/config/bitcoin-config.h.in | paste -sd "|" -)
The second command holds a list of files and directories that should not be processed. These include subtree directories as well as some crypto files that already get their symbols through the makefile.
The third command checks for missing bitcoin-config headers in .cpp files and adds the header if it is missing.
The fourth command checks for missing bitcoin-config headers in .h files and adds the header if it is missing.
The fifth command checks for unneeded bitcoin-config headers in sources files and removes the header if it is unneeded.
29029df5c7 [doc] v3 signaling in mempool-replacements.md (glozow)
e643ea795e [fuzz] v3 transactions and sigop-adjusted vsize (glozow)
1fd16b5c62 [functional test] v3 transaction submission (glozow)
27c8786ba9 test framework: Add and use option for tx-version in MiniWallet methods (MarcoFalke)
9a1fea55b2 [policy/validation] allow v3 transactions with certain restrictions (glozow)
eb8d5a2e7d [policy] add v3 policy rules (glozow)
9a29d470fb [rpc] return full string for package_msg and package-error (glozow)
158623b8e0 [refactor] change Workspace::m_conflicts and adjacent funcs/structs to use Txid (glozow)
Pull request description:
See #27463 for overall package relay tracking.
Delving Bitcoin discussion thread: https://delvingbitcoin.org/t/v3-transaction-policy-for-anti-pinning/340
Delving Bitcoin discussion for LN usage: https://delvingbitcoin.org/t/lightning-transactions-with-v3-and-ephemeral-anchors/418
Rationale:
- There are various pinning problems with RBF and our general ancestor/descendant limits. These policies help mitigate many pinning attacks and make package RBF feasible (see #28984 which implements package RBF on top of this). I would focus the most here on Rule 3 pinning. [1][2]
- Switching to a cluster-based mempool (see #27677 and #28676) requires the removal of CPFP carve out, which applications depend on. V3 + package RBF + ephemeral anchors + 1-parent-1-child package relay provides an intermediate solution.
V3 policy is for "Priority Transactions." [3][4] It allows users to opt in to more restrictive topological limits for shared transactions, in exchange for the more robust fee-bumping abilities that offers. Even though we don't have cluster limits, we are able to treat these transactions as having as having a maximum cluster size of 2.
Immediate benefits:
- You can presign a transaction with 0 fees (not just 1sat/vB!) and add a fee-bump later.
- Rule 3 pinning is reduced by a significant amount, since the attacker can only attach a maximum of 1000vB to your shared transaction.
This also enables some other cool things (again see #27463 for overall roadmap):
- Ephemeral Anchors
- Package RBF for these 1-parent-1-child packages. That means e.g. a commitment tx + child can replace another commitment tx using the child's fees.
- We can transition to a "single anchor" universe without worrying about package limit pinning. So current users of CPFP carve out would have something else to use.
- We can switch to a cluster-based mempool [5] (#27677#28676), which removes CPFP carve out [6].
[1]: Original mailing list post and discussion about RBF pinning problems https://gist.github.com/glozow/25d9662c52453bd08b4b4b1d3783b9ff, https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2022-January/019817.html
[2]: A FAQ is "we need this for cluster mempool, but is this still necessary afterwards?" There are some pinning issues that are fixed here and not fully fixed in cluster mempool, so we will still want this or something similar afterward.
[3]: Mailing list post for v3 https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2022-September/020937.html
[4]: Original PR #25038 also contains a lot of the discussion
[5]: https://delvingbitcoin.org/t/an-overview-of-the-cluster-mempool-proposal/393/7
[6]: https://delvingbitcoin.org/t/an-overview-of-the-cluster-mempool-proposal/393#the-cpfp-carveout-rule-can-no-longer-be-supported-12
ACKs for top commit:
sdaftuar:
ACK 29029df5c7
achow101:
ACK 29029df5c7
instagibbs:
ACK 29029df5c7 modulo that
Tree-SHA512: 9664b078890cfdca2a146439f8835c9d9ab483f43b30af8c7cd6962f09aa557fb1ce7689d5e130a2ec142235dbc8f21213881baa75241c5881660f9008d68450
ff9039f6ea Remove GetAdjustedTime (dergoegge)
Pull request description:
This picks up parts of #25908.
The use of adjusted time is removed from validation code while the warning to users if their clock is out of sync with the rest of the network remains.
ACKs for top commit:
naumenkogs:
ACK ff9039f6ea
achow101:
ACK ff9039f6ea
maflcko:
lgtm ACK ff9039f6ea🤽
stickies-v:
ACK ff9039f6ea
Tree-SHA512: d1f6b9445c236915503fd2ea828f0d3b92285a5dbc677b168453276115e349972edbad37194d8becd9136d8e7219b576af64ec51c72bdb1923e57e405c0483fc
These exceptions are not related to situations specific to tests,
but are required in general:
Without the first check CheckBlockindex could fail for blocks where we
only know the header.
Without the second, it could fail when blocks are received out of order.
cdc6ac4126 snapshots: don't core dump when running -checkblockindex after `loadtxoutset` (Mark Friedenbach)
Pull request description:
Transaction counts aren't known for block history loaded from a snapshot. If you start with `-checkblockindex` after loading a snapshot, the bitcoin daemon will core dump. The test suite does not check for this because all the snapshots have no non-coinbase transactions (all blocks prior to the snapshot are assumed to have `nTx = 1`).
Recommend for backport to 26.x
ACKs for top commit:
fjahr:
utACK cdc6ac4126
achow101:
ACK cdc6ac4126
pablomartin4btc:
tACK cdc6ac4126
Tree-SHA512: f7488a85cc29056e2ac443ce8f34aea4dfde6ba246efce82235d6a4dca2dca4344f07b93c93424b4addcb83e4cb2ae49a3ebb37d89840d42d2aeea35904cab04
It's preferable to use type-safe transaction identifiers to avoid
confusing txid and wtxid. The next commit will add a reference to this
set; we use this opportunity to change it to Txid ahead of time instead
of adding new uses of uint256.
Update CheckPackageLimits to use util::Result to pass the error message
instead of out parameter.
Also update test to reflect the error message from `CTxMempool`
`CheckPackageLimits` output.
91504cbe0d rpc: `SyncWithValidationInterfaceQueue` on fee estimation RPC's (ismaelsadeeq)
714523918b tx fees, policy: CBlockPolicyEstimator update from `CValidationInterface` notifications (ismaelsadeeq)
dff5ad3b99 CValidationInterface: modify the parameter of `TransactionAddedToMempool` (ismaelsadeeq)
91532bd382 tx fees, policy: update `CBlockPolicyEstimator::processBlock` parameter (ismaelsadeeq)
bfcd401368 CValidationInterface, mempool: add new callback to `CValidationInterface` (ismaelsadeeq)
0889e07987 tx fees, policy: cast with static_cast instead of C-Style cast (ismaelsadeeq)
a0e3eb7549 tx fees, policy: bugfix: move `removeTx` into reason != `BLOCK` condition (ismaelsadeeq)
Pull request description:
This is an attempt to #11775
This Pr will enable fee estimator to listen to ValidationInterface notifications to process new transactions added and removed from the mempool.
This PR includes the following changes:
- Added a new callback to the Validation Interface `MempoolTransactionsRemovedForConnectedBlock`, which notifies listeners about the transactions that have been removed due to a new block being connected, along with the height at which the transactions were removed.
- Modified the `TransactionAddedToMempool` callback parameter to include additional information about the transaction needed for fee estimation.
- Updated `CBlockPolicyEstimator` to process transactions using` CTransactionRef` instead of `CTxMempoolEntry.`
- Implemented the `CValidationInterface` interface in `CBlockPolicyEstimater` and overridden the `TransactionAddedToMempool`, `TransactionRemovedFromMempool`, and `MempoolTransactionsRemovedForConnectedBlock` methods to receive updates from their notifications.
Prior to this PR, the fee estimator updates from the mempool, i.e whenever a new block is connected all transactions in the block that are in our mempool are going to be removed using the `removeForBlock` function in `txmempool.cpp`.
This removal triggered updates to the fee estimator. As a result, the fee estimator would block mempool's `cs` until it finished updating every time a new block was connected.
Instead of being blocked only on mempool tx removal, we were blocking on both tx removal and fee estimator updating.
If we want to further improve fee estimation, or add heavy-calulation steps to it, it is currently not viable as we would be slowing down block relay in the process
This PR is smaller in terms of the changes made compared to #11775, as it focuses solely on enabling fee estimator updates from the validationInterface/cscheduler thread notifications.
I have not split the validation interface because, as I understand it, the rationale behind the split in #11775 was to have `MempoolInterface` signals come from the mempool and `CValidationInterface` events come from validation. I believe this separation can be achieved in a separate refactoring PR when the need arises.
Also left out some commits from #11775
- Some refactoring which are no longer needed.
- Handle reorgs much better in fee estimator.
- Track witness hash malleation in fee estimator
I believe they are a separate change that can come in a follow-up after this.
ACKs for top commit:
achow101:
ACK 91504cbe0d
TheCharlatan:
Re-ACK 91504cbe0d
willcl-ark:
ACK 91504cbe0d
Tree-SHA512: 846dfb9da57a8a42458827b8975722d153907fe6302ad65748d74f311e1925557ad951c3d95fe71fb90ddcc8a3710c45abb343ab86b88780871cb9c38c72c7b1
fa1a384706 Move compat.h include from system.h to system.cpp (MarcoFalke)
88887531b7 Move compat/assumptions.h include to one place that actually needs it (MarcoFalke)
77774110f4 Remove __cplusplus from compat/assumptions.h (MarcoFalke)
faa3d4f1d8 Remove duplicate NDEBUG check from compat/assumptions.h (MarcoFalke)
Pull request description:
Generally, compile-time checks should be close to the code that use them. Especially, since `compat/assumptions.h` is only included in one place, where iwyu suggests to remove it.
Fix all issues:
* The `NDEBUG` check is used in `util/check`, so it is redundant in `compat/assumptions.h`.
* The `__cplusplus` check is redundant with `doc/dependencies.md` (see commit message).
* Add missing `// IWYU pragma: keep` to avoid removing the include by accident.
ACKs for top commit:
achow101:
ACK fa1a384706
TheCharlatan:
re-ACK fa1a384706
theuni:
ACK fa1a384706
Tree-SHA512: f8b6db84be5d8844a2267345c0b1405fcbc39b8b5eeaa24db5b8412a74145fe44cf188b6b0c39cc2b062690ed37ca5b4662473484afe28dbec6469e79961389b
9e58c5bcd9 Use Txid in COutpoint (dergoegge)
Pull request description:
This PR changes the type of the hash of a transaction outpoint from `uint256` to `Txid`.
ACKs for top commit:
Sjors:
ACK 9e58c5bcd9
stickies-v:
ACK 9e58c5bcd9. A sizeable diff, but very straightforward changes. Didn't see anything controversial. Left a few nits, but nothing blocking, only if you have to retouch.
TheCharlatan:
ACK 9e58c5bcd9
Tree-SHA512: 58f61ce1c58668f689513e62072a7775419c4d5af8f607669cd8cdc2e7be9645ba14af7f9e2d65da2670da3ec1ce7fc2a744037520caf799aba212fd1ac44b34
`CBlockPolicyEstimator` will implement `CValidationInterface` and
subscribe to its notification to process transactions added and removed
from the mempool.
Re-delegate calculation of `validForFeeEstimation` from validation to fee estimator.
Also clean up the validForFeeEstimation arg thats no longer needed in `CTxMempool`.
Co-authored-by: Matt Corallo <git@bluematt.me>
4dd94ca18f [refactor] remove access to mapTx in validation_block_tests (TheCharlatan)
d0cd2e804e [refactor] rewrite BlockAssembler inBlock and failedTx as sets of txids (glozow)
55b0939cab scripted-diff: rename vTxHashes to txns_randomized (TheCharlatan)
a03aef9cec [refactor] rewrite vTxHashes as a vector of CTransactionRef (glozow)
938643c3b2 [refactor] remove access to mapTx in validation.cpp (glozow)
333367a940 [txmempool] make CTxMemPoolEntry::lockPoints mutable (glozow)
1bf4855016 [refactor] use CheckPackageLimits for checkChainLimits (glozow)
dbc5bdbf59 [refactor] remove access to mapTx.find in mempool_tests.cpp (glozow)
f80909e7a3 [refactor] remove access to mapTx in blockencodings_tests.cpp (glozow)
8892d6b744 [refactor] remove access to mapTx from rpc/mempool.cpp (glozow)
fad61aa561 [refactor] get wtxid from entry instead of vTxHashes (glozow)
9cd8cafb77 [refactor] use exists() instead of mapTx.find() (glozow)
14804699e5 [refactor] remove access to mapTx from policy/rbf.cpp (glozow)
1c6a73abbd [refactor] Add helper for retrieving mempool entry (TheCharlatan)
453b4813eb [refactor] Add helper for iterating through mempool entries (stickies-v)
Pull request description:
Motivation
* It seems preferable to use stdlib data structures instead of boost if they can achieve close to the same thing.
* Code external to mempool should ideally use its public helper methods instead of accessing `mapTx` or its iterators directly.
* Reduce the number of complex boost multi index type interactions
* Also see #28335 for further context/motivation. This PR together with #28385 simplifies that one.
Overview of things done in this PR:
* Make `vTxHashes` a vector of transaction references instead of a pair of transaction hash and iterator. The trade off here is that the data is retrieved on the fly with `GetEntry` instead of being cached in `vTxHashes`.
* Introduce `GetEntry` helper method to replace the more involved `GetIter` where applicable
* Replace `mapTx` access with `CTxMemPool` helper methods
* Simplify `checkChainLimits` call in `node/interfaces.cpp`
* Make `CTxMemPoolEntry`s `lockPoints`mutable such that they can be changed with a const iterator directly instead of going through `mapTx`
* Make `BlockAssembler`'s `inBlock` and `failedTx` sets of transaction hashes.
ACKs for top commit:
glozow:
reACK 4dd94ca
maflcko:
re-ACK 4dd94ca18f👝
stickies-v:
re-ACK 4dd94ca18f
Tree-SHA512: c4d043f2186e4fde337591883fac66cade3058173987b49502bd65cecf69207a3df1077f6626809652ab63230013167b7f39a2b39f1c5166959e5495df57065f
With subpackage evaluation and de-duplication, it's not always the
entire package that is used in CheckFeerate. To be more helpful to the
caller, specify which transactions were included in the evaluation and
what the feerate was.
Instead of PCKG_POLICY (which is supposed to be for package-wide
errors), use PCKG_TX.
With package validation rules, transactions that fail individually may
sometimes be eligible for reconsideration if submitted as part of a
(different) package. For now, that includes trasactions that failed for
being too low feerate. Add a new TxValidationResult type to distinguish
these failures from others. In the next commits, we will abort package
validation if a tx fails for any other reason. In the future, we will
also decide whether to cache failures in recent_rejects based on this
result (we won't want to reject a package containing a transaction that
was rejected previously for being low feerate).
Package validation also sometimes elects to skip some transactions when
it knows the package will not be submitted in order to quit sooner. Add
a result to specify this situation; we also don't want to cache these
as rejections.
b5a60abe87 MOVEONLY: CleanupTemporaryCoins into its own function (glozow)
10c0a8678c [test util] CreateValidTransaction multi-in/out, configurable feerate, signal BIP125 (glozow)
6ff647a7e0 scripted-diff: rename CheckPackage to IsWellFormedPackage (glozow)
da9aceba21 [refactor] move package checks into helper functions (glozow)
Pull request description:
This is part of #27463. It splits off the more trivial changes from #26711 for ease of review, as requested in https://github.com/bitcoin/bitcoin/pull/26711#issuecomment-1786392253.
- Split package sanitization in policy/packages.h into helper functions
- Add some tests for its quirks (https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1340521597)
- Rename `CheckPackage` to `IsPackageWellFormed`
- Improve the `CreateValidTransaction` unit test utility to:
- Configure the target feerate and return the fee paid
- Signal BIP125 on transactions to enable RBF tests
- Allow the specification of multiple inputs and outputs
- Move `CleanupTemporaryCoins` into its own function to be reused later without duplication
ACKs for top commit:
dergoegge:
Code review ACK b5a60abe87
instagibbs:
ACK b5a60abe87
Tree-SHA512: 39d67a5f0041e381f0d0f802a98ccffbff11e44daa3a49611189d6306b03f18613d5ff16c618898d490c97a216753e99e0db231ff14d327f92c17ae4d269cfec
9b3da70bd0 [test] DisconnectedBlockTransactions::DynamicMemoryUsage (glozow)
b2d0447964 bugfix: correct DisconnectedBlockTransactions memory usage (stickies-v)
f4254e2098 assume duplicate transactions are not added to `iters_by_txid` (ismaelsadeeq)
29eb219c12 move only: move implementation code to disconnected_transactions.cpp (ismaelsadeeq)
81dfeddea7 refactor: update `MAX_DISCONNECTED_TX_POOL` from kb to bytes (ismaelsadeeq)
Pull request description:
This PR is a follow-up to fix review comments and a bugfix from #28385
The PR
- Updated `DisconnectedBlockTransactions`'s `MAX_DISCONNECTED_TX_POOL` from kb to bytes.
- Moved `DisconnectedBlockTransactions` implementation code to `kernel/disconnected_transactions.cpp`.
- `AddTransactionsFromBlock` now assume duplicate transactions are not passed by asserting after inserting each transaction to `iters_by_txid`.
- Included a Bug fix: In the current master we are underestimating the memory usage of `DisconnectedBlockTransactions`.
* When adding and subtracting `cachedInnerUsage` we call `RecursiveDynamicUsage` with `CTransaction` which invokes this [`RecursiveDynamicUsage(const CTransaction& tx)`](6e721c923c/src/core_memusage.h (L32)) version of `RecursiveDynamicUsage`, the output of that call only account for the memory usage of the inputs and outputs of the `CTransaction`, this omits the memory usage of the `CTransaction` object and the control block.
* This PR fixes this bug by calling `RecursiveDynamicUsage` with `CTransactionRef` when adding and subtracting `cachedInnerUsage` which invokes [`RecursiveDynamicUsage(const std::shared_ptr<X>& p)`](6e721c923c/src/core_memusage.h (L67)) version of `RecursiveDynamicUsage` the output of the calculation accounts for the` CTransaction` object, the control blocks, inputs and outputs memory usage.
* see [comment ](https://github.com/bitcoin/bitcoin/pull/28385#discussion_r1322948452)
- Added test for DisconnectedBlockTransactions memory limit.
ACKs for top commit:
stickies-v:
ACK 9b3da70bd0 - nice work!
BrandonOdiwuor:
re ACK 9b3da70bd0
glozow:
ACK 9b3da70bd0
Tree-SHA512: 69b9595d09f4d0209038f97081d790cea92ccf63efb94e9e372749979fcbe527f7f17a8e454720cedd12021be0c8e11cf99874625d3dafd9ec602b12dbeb4098
This allows IsSorted() and IsConsistent() to be used by themselves.
IsSorted() with a precomputed set is used so that we don't create this
set multiple times.
faa769db5a Fix bugprone-lambda-function-name errors (MarcoFalke)
Pull request description:
Inside a lambda, `__func__` will evaluate to something like `"operator()"`. Fix this by either removing it, or by using the real name.
https://clang.llvm.org/extra/clang-tidy/checks/bugprone/lambda-function-name.html
ACKs for top commit:
TheCharlatan:
ACK faa769db5a
darosior:
utACK faa769db5a
Tree-SHA512: 0b562bd4ebd7f46ca3ebabeee67851ad30bd522fa57e5010e833b163664e51f5df645ff9ca35d22c3479fb27d9267d4e5d0d417d42729bf3ccf80d7944970e4e
940a49978c Use type-safe txid types in orphanage (dergoegge)
ed70e65016 Introduce types for txids & wtxids (dergoegge)
cdb14d79e8 [net processing] Use HasWitness over comparing (w)txids (dergoegge)
Pull request description:
We currently have two different identifiers for transactions: `txid` (refering to the hash of a transaction without witness data) and `wtxid` (referring to the hash of a transaction including witness data). Both are typed as `uint256` which could lead to type-safety bugs in which one transaction identifier type is passed where the other would be expected.
This PR introduces explicit `Txid` and `Wtxid` types that (if used) would cause compilation errors for such type confusion bugs.
(Only the orphanage is converted to use these types in this PR)
ACKs for top commit:
achow101:
ACK 940a49978c
stickies-v:
ACK 940a49978c
hebasto:
ACK 940a49978c, I have reviewed the code and it looks OK.
instagibbs:
re-ACK 940a49978c
BrandonOdiwuor:
re-ACK 940a49978c
glozow:
reACK 940a49978c
Tree-SHA512: 55298d1c2bb82b7a6995e96e554571c22eaf4a89fb2a4d7a236d70e0f625e8cca62ff2490e1c179c47bd93153fe6527b56870198f026f5ee7753d64d7a424c92
ec84f999f1 log: Don't log cache rebalancing in absense of a snapshot chainstate (Fabian Jahr)
Pull request description:
I have noticed that this log now is always printed, even if there is no snapshot chainstate present or even was present. I think this is confusing to users that have never even thought about using assumeutxo since in that case the rebalancing is just ensuring the normal environment with one chainstate. So I suggest we don't log in absence of a snapshot chainstate. We could also think about rewording the message instead but I think this is simpler.
ACKs for top commit:
stickies-v:
utACK ec84f999f1
glozow:
concept ACK ec84f999f1, don't have opinions other than removing confusing log
theStack:
utACK ec84f999f1
Tree-SHA512: 30bbfc648e7c788106f78d52e47a3aa1e1874f65d13743643dc50bcf7f450d8330711ff9fdeac361722542da6051533153829c6d49033227ed315e111afc899f
5b878be742 [doc] add release note for submitpackage (glozow)
7a9bb2a2a5 [rpc] allow submitpackage to be called outside of regtest (glozow)
5b9087a9a7 [rpc] require package to be a tree in submitpackage (glozow)
e32ba1599c [txpackages] IsChildWithParentsTree() (glozow)
b4f28cc345 [doc] parent pay for child in aggregate CheckFeeRate (glozow)
Pull request description:
Permit (restricted topology) submitpackage RPC outside of regtest. Suggested in https://github.com/bitcoin/bitcoin/pull/26933#issuecomment-1510851570
This RPC should be safe but still experimental - interface may change, not all features (e.g. package RBF) are implemented, etc. If a miner wants to expose this to people, they can effectively use "package relay" before the p2p changes are implemented. However, please note **this is not package relay**; transactions submitted this way will not relay to other nodes if the feerates are below their mempool min fee. Users should put this behind some kind of rate limit or permissions.
ACKs for top commit:
instagibbs:
ACK 5b878be742
achow101:
ACK 5b878be742
dergoegge:
Code review ACK 5b878be742
ajtowns:
ACK 5b878be742
ariard:
Code Review ACK 5b878be742. Though didn’t manually test the PR.
Tree-SHA512: 610365c0b2ffcccd55dedd1151879c82de1027e3319712bcb11d54f2467afaae4d05dca5f4b25f03354c80845fef538d3938b958174dda8b14c10670537a6524
This ensures that we avoid any unexpected conditions inherent in
transferring non-empty mempools across chainstates.
Note that this should never happen in practice given that snapshot
activation will not occur outside of IBD, based upon the height checks
in `loadtxoutset`.
When using an assumedvalid (snapshot) chainstate along with a background
chainstate, we are syncing two very different regions of the chain
simultaneously. If we use the same blockfile space for both of these
syncs, wildly different height blocks will be stored alongside one
another, making pruning ineffective.
This change implements a separate blockfile cursor for the assumedvalid
chainstate when one is in use.
Use the expected AssumeutxoData in order to bootstrap nChainTx values
for assumedvalid blockindex entries in the snapshot chainstate. This
is necessary because nChainTx is normally built up from nTx values,
which are populated using blockdata which the snapshot chainstate
does not yet have.
