| Age | Commit message (Collapse) | Author |
|---|
|
This makes a number of changes:
- Get rid of the verification_progress argument in the node interface
NotifyHeaderTip (it was always 0.0).
- Instead of passing a CBlockIndex* in the UI interface's NotifyHeaderTip,
send separate height, timestamp fields. This is becuase in headers presync,
no actual CBlockIndex object is available.
- Add a bool presync argument to both of the above, to identify signals
pertaining to the first headers sync phase.
|
|
|
|
In order to prevent memory DoS, we must ensure that we don't accept a new
header into memory until we've performed anti-DoS checks, such as verifying
that the header is part of a sufficiently high work chain. This commit adds a
new argument to AcceptBlockHeader() so that we can ensure that all call-sites
which might cause a new header to be accepted into memory have to grapple with
the question of whether the header is safe to accept, or needs further
validation.
This patch also fixes two places where low-difficulty-headers could have been
processed without such validation (processing an unrequested block from the
network, and processing a compact block).
Credit to Niklas Gögge for noticing this issue, and thanks to Sjors Provoost
for test code.
|
|
Avoid permanently storing headers from a peer, unless the headers are part of a
chain with sufficiently high work. This prevents memory attacks using low-work
headers.
Designed and co-authored with Pieter Wuille.
|
|
1dc03dda05e9dce128e57f05bb7b1bb02b3cfb9e [doc] remove non-signaling mentions of BIP125 (glozow)
32024d40f03fbf47c64d814fa5f2c2a73ec14cb7 scripted-diff: remove mention of BIP125 from non-signaling var names (glozow)
Pull request description:
We have pretty thorough documentation of our RBF policy in doc/policy/mempool-replacements.md. It enumerates each rule with several sentences of rationale. Also, each rule pretty much has its own function (3 and 4 share one), with extensive comments. The doc states explicitly that our rules are similar but differ from BIP125, and contains a record of historical changes to RBF policy.
We should not use "BIP125" as synonymous with our RBF policy because:
- Our RBF policy is different from what is specified in BIP125, for example:
- the BIP does not mention our rule about the replacement feerate being higher (our Rule 6)
- the BIP uses minimum relay feerate for Rule 4, while we have used incremental relay feerate since #9380
- the "inherited signaling" question (CVE-2021-31876). Call it discrepancy, ambiguous wording, doc misinterpretation, or implementation details, I would recommend users refer to doc/policy/mempool-replacements.md
- the signaling policy is configurable, see #25353
- Our RBF policy may change further
- We have already marked BIP125 as only "partially implemented" in docs/bips.md since 1fd49eb498c75a1d14193bb736d195a3dc75ae12
- See comments from people who are not me recently:
- https://github.com/bitcoin/bitcoin/pull/25038#discussion_r909507429
- https://github.com/bitcoin/bitcoin/pull/25575#issuecomment-1179519204
This PR removes all non-signaling mentions of BIP125 (if people feel strongly, we can remove all mentions of BIP125 period). It may be useful to refer to the concept of "tx opts in to RBF if it has at least one nSequence less than (0xffffffff - 1)" as "BIP125 signaling" because:
- It is succint.
- It has already been widely marketed as BIP125 opt-in signaling.
- Our API uses it when referring to signaling (e.g. getmempoolentry["bip125-replaceable"] and wallet error message "not BIP 125 replaceable"). Changing those is more invasive.
- If/when we have other ways to signal in the future, we can disambiguate them this way. See #25038 which proposes another way of signaling, and where I pulled these commits from.
Alternatives:
- Changing our policy to match BIP125. This doesn't make sense as, for example, we would have to remove the requirement that a replacement tx has a higher feerate (Rule 6).
- Changing BIP125 to match what we have. This doesn't make sense as it would be a significant change to a BIP years after it was finalized and already used as a spec to implement RBF in other places.
- Document our policy as a new BIP and give it a number. This might make sense if we don't expect things to change a lot, and can be done as a next step.
ACKs for top commit:
darosior:
ACK 1dc03dda05e9dce128e57f05bb7b1bb02b3cfb9e
ariard:
ACK 1dc03dda
t-bast:
ACK https://github.com/bitcoin/bitcoin/commit/1dc03dda05e9dce128e57f05bb7b1bb02b3cfb9e
Tree-SHA512: a3adc2039ec5785892d230ec442e50f47f7062717392728152bbbe27ce1c564141f85253143f53cb44e1331cf47476d74f5d2f4b3cd873fc3433d7a0aa783e02
|
|
eeee5ada23f2a71d245671556b6ecfdaabfeddf4 Make adjusted time type safe (MacroFake)
fa3be799fe951a7ea9b4de78d5a907c6db71eeb8 Add time helpers (MacroFake)
Pull request description:
This makes follow-ups easier to review. Also, it makes sense by itself.
ACKs for top commit:
ryanofsky:
Code review ACK eeee5ada23f2a71d245671556b6ecfdaabfeddf4. Confirmed type changes and equivalent code changes only.
Tree-SHA512: 51bf1ae5428552177286113babdd49e82459d6c710a07b6e80a0a045d373cf51045ee010461aba98e0151d8d71b9b3b5f8f73e302d46ba4558e0b55201f99e9f
|
|
9376a6dae41022874df3b9302667796a9fb7b81d refactor: make active_chain_tip a reference (Aurèle Oulès)
Pull request description:
This PR fixes a TODO introduced in #21055.
Makes `active_chain_tip` argument in `CheckFinalTxAtTip` function a reference instead of a pointer.
ACKs for top commit:
dongcarl:
ACK 9376a6dae41022874df3b9302667796a9fb7b81d
Tree-SHA512: c36d1769e0b9598b7f79334704b26b73e958d54caa3bd7e4eff954f3964fcf3f5e3a44a5a760497afad51b76e1614c86314fe035e4083c855e3574a620de7f4d
|
|
|
|
Our RBF policy is different from the rules specified in BIP125. For
example, the BIP does not mention Rule 6, and our Rule 4 uses the
(configurable) incremental relay feerate (distinct from the
minimum relay feerate). Those interested in our policy should refer to
doc/policy/mempool-replacements.md instead. These rules may also
continue to diverge with package RBF and other RBF improvements. Keep
references to the BIP125 signaling wrt sequence numbers, since that is
still correct and widely used. It is helpful to refer to this as "BIP125
signaling" since it is unambiguous and succint, especially if we have
multiple ways to signal replaceability in the future.
The rule numbers in doc/policy/mempool-replacements.md correspond
largely to those of BIP 125, so we can still refer to them like "Rule 5."
|
|
faab8dceb37a944d0763fcca390342111e6a9fcc Remove unused SetTip(nullptr) code (MacroFake)
Pull request description:
Now that this path is no longer used after commit b51e60f91472da5216116626afc032acd5616e85, we can remove it.
Future code should reset `CChain` by simply discarding it and constructing a fresh one.
ACKs for top commit:
ryanofsky:
Code review ACK faab8dceb37a944d0763fcca390342111e6a9fcc. Just moved an assert statement since last review
Tree-SHA512: 7dc273b11133d85d32ca2a69c0c7c07b39cdd338141ef5b51496e7de334a809864d5459eb95535497866c8b1e468aae84ed8f91b543041e6ee20130d5622874e
|
|
...also move the 0-clamping logic to ApplyArgsManOptions, where it
belongs.
|
|
Also:
- Make DEFAULT_MAX_SIG_CACHE_SIZE into constexpr
DEFAULT_MAX_SIG_CACHE_BYTES to utilize the compile-time integer
arithmetic overflow checking available to constexpr.
- Fix comment (MiB instead of MB) for DEFAULT_MAX_SIG_CACHE_BYTES.
- Pass in max_size_bytes parameter to InitS*Cache(), modify log line to
no longer allude to maxsigcachesize being split evenly between the two
validation caches.
- Fix possible integer truncation and add a comment.
[META] I've kept the integer types as int64_t in order to not introduce
unintended behaviour changes, in the next commit we will make
them size_t.
|
|
This fixes an potential overflow which existed prior to this patchset.
If CuckooCache::cache<Element, Hash>::setup_bytes is called with a
`size_t bytes` which, when divided by sizeof(Element), does not fit into
an uint32_t, the implicit conversion to uint32_t in the call to setup
will result in an overflow.
At least on x86_64, this overflow is possible:
static_assert(std::numeric_limits<size_t>::max() / 32 <= std::numeric_limits<uint32_t>::max());
static_assert(std::numeric_limits<size_t>::max() / 4 <= std::numeric_limits<uint32_t>::max());
This commit detects such cases and signals to callers that the `size_t
bytes` input is too large.
|
|
1. -maxsigcachesize is a DEBUG_ONLY option
2. Almost 7 years has passed since its semantics change in
830e3f3d027ba5c8121eed0f6a9ce99961352572 from "number of entries" to
"number of mebibytes"
3. A std::new_handler was added to the codebase after the original PR
which introduced this limit, which will terminate immediately instead
of causing trouble by being caught somewhere unexpected.
|
|
Returning the approximate total size eliminates the need for
InitS*Cache() to do nElems*sizeof(uint256). The cuckoocache has a better
idea of this information.
|
|
Our RBF policy is different from the rules specified in BIP125 (refer to
doc/policy/mempool-replacements.md instead), and will continue to
diverge with package RBF. Keep references to BIP125 sequence number,
since that is still useful and correct.
-BEGIN VERIFY SCRIPT-
ren() { sed -i "s:\<$1\>:$2:g" $(git grep -l "\<$1\>" ./src ./test); }
ren m_allow_bip125_replacement m_allow_replacement
ren allow_bip125_replacement allow_replacement
ren MAX_BIP125_REPLACEMENT_CANDIDATES MAX_REPLACEMENT_CANDIDATES
-END VERIFY SCRIPT-
|
|
|
|
|
|
|
|
|
|
Apart from tests, it is only used in one place, so there is no need for
an alias.
|
|
|
|
|
|
and rename it
dd065dae9fcebd6806ff67703ffa8128e80b97cc refactor: Make mapBlocksUnknownParent local, and rename it (Hennadii Stepanov)
Pull request description:
This PR is a second attempt at #19594. This PR has two motivations:
- Improve code hygiene by eliminating a global variable, `mapBlocksUnknownParent`
- Fix fuzz test OOM when running too long ([see #19594 comment](https://github.com/bitcoin/bitcoin/pull/19594#issuecomment-958801638))
A minor added advantage is to release `mapBlocksUnknownParent` memory when the reindexing phase is done. The current situation is somewhat similar to a memory leak because this map exists unused for the remaining lifetime of the process. It's true that this map should be empty of data elements after use, but its internal metadata (indexing structures, etc.) can have non-trivial size because there can be many thousands of simultaneous elements in this map.
This PR helps our efforts to reduce the use of global variables. This variable isn't just global, it's hidden inside a function (it looks like a local variable but has the `static` attribute).
This global variable exists because the `-reindex` processing code calls `LoadExternalBlockFile()` multiple times (once for each block file), but that function must preserve some state between calls (the `mapBlocksUnknownParent` map). This PR fixes this by allocating this map as a local variable in the caller's scope and passing it in on each call. When reindexing completes, the map goes out of scope and is deallocated.
I tested this manually by reindexing on mainnet and signet. Also, the existing `feature_reindex.py` functional test passes.
ACKs for top commit:
mzumsande:
re-ACK dd065dae9fcebd6806ff67703ffa8128e80b97cc
theStack:
re-ACK dd065dae9fcebd6806ff67703ffa8128e80b97cc
shaavan:
reACK dd065dae9fcebd6806ff67703ffa8128e80b97cc
Tree-SHA512: 9cd20e44d2fa1096dd405bc107bc065ea8f904f5b3f63080341b08d8cf57b790df565f58815c2f331377d044d5306708b4bf6bdfc5ef8d0ed85d8e97d744732c
|
|
safety, consistent behavior
3a61fc56a0ad6ed58570350dcfd9ed2d10239b48 refactor: move CBlockIndex#ToString() from header to implementation (Jon Atack)
57865eb51288852c3ce99607eff76c61ae5f5365 CDiskBlockIndex: rename GetBlockHash() to ConstructBlockHash() (Jon Atack)
99e8ec8721a52cd08bdca31f6e926c9c1ce281fb CDiskBlockIndex: remove unused ToString() class member (Jon Atack)
14aeece462b149eaf0d28a37d55cc169df99b2cb CBlockIndex: ensure phashBlock is not nullptr before dereferencing (Jon Atack)
Pull request description:
Fix a few design issues, potential footguns and inconsistent behavior in the CBlockIndex and CDiskBlockIndex classes.
- Ensure phashBlock in `CBlockIndex#GetBlockHash()` is not nullptr before dereferencing and remove a now-redundant assert preceding a GetBlockHash() caller. This protects against UB here, and in case of failure (which would indicate a consensus bug), the debug log will print `bitcoind: chain.h:265: uint256 CBlockIndex::GetBlockHash() const: Assertion 'phashBlock != nullptr' failed. Aborted` instead of `Segmentation fault`.
- Remove the unused `CDiskBlockIndex#ToString()` class member, and mark the inherited `CBlockIndex#ToString()` public interface member as deleted to disallow calling it in the derived CDiskBlockIndex class.
- Rename the `CDiskBlockIndex GetBlockHash()` class member to `ConstructBlockHash()`, which also makes sense as they perform different operations to return a blockhash, and mark the inherited `CBlockIndex#GetBlockHash()` public interface member as deleted to disallow calling it in the derived CDiskBlockIndex class.
- Move `CBlockIndex#ToString()` from header to implementation, which also allows dropping `tinyformat.h` from the header file.
Rationale and discussion regarding the CDiskBlockIndex changes:
Here is a failing test on master that demonstrates the inconsistent behavior of the current design: calling the same inherited public interface functions on the same CDiskBlockIndex object should yield identical behavior, but does not.
```diff
diff --git a/src/test/validation_chainstatemanager_tests.cpp b/src/test/validation_chainstatemanager_tests.cpp
index 6dc522b421..dac3840f32 100644
--- a/src/test/validation_chainstatemanager_tests.cpp
+++ b/src/test/validation_chainstatemanager_tests.cpp
@@ -240,6 +240,15 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_activate_snapshot, TestChain100Setup)
const CBlockIndex* tip = chainman.ActiveTip();
BOOST_CHECK_EQUAL(tip->nChainTx, au_data.nChainTx);
+ // CDiskBlockIndex "is a" CBlockIndex, as it publicly inherits from it.
+ // Test that calling the same inherited interface functions on the same
+ // object yields identical behavior.
+ CDiskBlockIndex index{tip};
+ CBlockIndex *pB = &index;
+ CDiskBlockIndex *pD = &index;
+ BOOST_CHECK_EQUAL(pB->GetBlockHash(), pD->GetBlockHash());
+ BOOST_CHECK_EQUAL(pB->ToString(), pD->ToString());
```
(build and run: `$ ./src/test/test_bitcoin -t validation_chainstatemanager_tests`)
The GetBlockHash() test assertion only passes on master because the different methods invoked by the current design happen to return the same result. If one of the two is changed, it fails like the ToString() assertion does.
Redefining inherited non-virtual functions is well-documented as incorrect design to avoid inconsistent behavior (see Scott Meyers, Effective C++, Item 36). Class usage is confusing when the behavior depends on the pointer definition instead of the object definition (static binding happening where dynamic binding was expected). This can lead to unsuspected or hard-to-track bugs.
Outside of critical hot spots, correctness usually comes before optimisation, but the current design dates back to main.cpp and it may possibly have been chosen to avoid the overhead of dynamic dispatch. This solution does the same: the class sizes are unchanged and no vptr or vtbl is added.
There are better designs for doing this that use composition instead of inheritance, or that separate the public interface from the private implementations. One example of the latter would be a non-virtual public interface that calls private virtual implementation methods, i.e. the Template pattern via the Non-Virtual Interface (NVI) idiom.
ACKs for top commit:
vasild:
ACK 3a61fc56a0ad6ed58570350dcfd9ed2d10239b48
Tree-SHA512: 9ff358ab0a6d010b8f053ad8303c6d4d061e62d9c3755a56b9c9f5eab855d02f02bee42acc77dfa0cbf4bb5cb775daa72d675e1560610a29bd285c46faa85ab7
|
|
|
|
and remove a now-redundant assert preceding a GetBlockHash() caller.
This protects against UB here, and in case of failure (which would
indicate a consensus bug), the debug log will print
bitcoind: chain.h:265: uint256 CBlockIndex::GetBlockHash() const: Assertion `phashBlock != nullptr' failed.
Aborted
instead of
Segmentation fault
|
|
and use it where possible
facc2fa7b8a218a0df6a19772e1641ea68dda2e3 Use AutoFile where possible (MacroFake)
6666803c897e4ad27b45cb74e3a9aa74a335f1bf streams: Add AutoFile without ser-type and ser-version (MacroFake)
Pull request description:
This was done in the context of https://github.com/bitcoin/bitcoin/pull/25284 , but I think it also makes sense standalone.
The basic idea is that serialization type should not be initialized when it is not needed. Same for the serialization version.
So do this here for `AutoFile`. `CAutoFile` remains in places where it is not yet possible.
ACKs for top commit:
laanwj:
Code review ACK facc2fa7b8a218a0df6a19772e1641ea68dda2e3
fanquake:
ACK facc2fa7b8a218a0df6a19772e1641ea68dda2e3
Tree-SHA512: d82d024d55af57565ac53d9d1517afafc12b46964effba0332de62a6c77869356fa77f89e6d4834438fff44c45b64fccdf5a1358bfea03e28dfe55013b3c099d
|
|
Co-authored-by: Larry Ruane <larryruane@gmail.com>
|
|
The following were unused from the node namespace:
- BLOCKFILE_CHUNK_SIZE
- nPruneTarget
- OpenBlockFile
- UNDOFILE_CHUNK_SIZE
|
|
Also:
1. Add the newly introduced kernel/mempool_persist.cpp to IWYU CI script
2. Add chrono mapping for iwyu
|
|
Also:
1. Have CChainState::LoadMempool and ::ThreadImport take in paths and
pass it through untouched to LoadMempool.
2. Make LoadMempool exit early if the load_path is empty.
3. Adjust the call to ::ThreadImport in ::AppInitMain to correctly pass
in an empty path if mempool persistence is disabled.
|
|
Not only does this increase coverage, it is also more correct in that
when ::LoadMempool is called with a mempool and chainstate, it calls
AcceptToMemoryPool with just the chainstate.
AcceptToMemoryPool will then act on the chainstate's mempool via
CChainState::GetMempool, which may be different from the mempool
originally passed to ::LoadMempool. (In this fuzz test's case, it
definitely is different)
Also, move DummyChainstate to its own file since it's now used by the
validation_load_mempool fuzz test to replace CChainState's m_mempool.
|
|
[META] In a future commit in this patchset, it will be used by more than
just validation, and it needs to align with fopen anyway.
|
|
|
|
m_is_loaded/IsLoaded() doesn't actually indicate whether or not the
mempool was successfully, loaded, but rather if a load has been
attempted and did not result in a catastrophic ShutdownRequested.
-BEGIN VERIFY SCRIPT-
find_regex="\bm_is_loaded\b" \
&& git grep -l -E "$find_regex" \
| xargs sed -i -E "s@$find_regex@m_load_tried@g"
find_regex="\bIsLoaded\b" \
&& git grep -l -E "$find_regex" \
| xargs sed -i -E "s@$find_regex@GetLoadTried@g"
find_regex="\bSetIsLoaded\b" \
&& git grep -l -E "$find_regex" \
| xargs sed -i -E "s@$find_regex@SetLoadTried@g"
-END VERIFY SCRIPT-
|
|
Also introduce node::{ShouldPersistMempool,MempoolPath} helper functions
in node/mempool_persist_args.{h,cpp} which are used by non-kernel
DumpMempool callers to determine whether or not to automatically dump
the mempool and where to dump it to.
|
|
This makes it so that DumpMempool doesn't depend on MICRO anymore
|
|
This new node policy setting enables to accept replaced-by-fee
transaction without inspection of the replaceability signaling
as described in BIP125 "explicit signaling".
If turns on, the node mempool accepts transaction replacement
as described in `policy/mempool-replacements.md`.
The default setting value is `false`, implying opt-in RBF
is enforced.
|
|
|
|
`ArgsManager`
d1684beabe5b738c2cc83de83e1aaef11a761b69 fees: Pass in a filepath instead of referencing gArgs (Carl Dong)
9a3d825c30e8e6118d74a4e568744cb9d03f7f5d init: Remove redundant -*mempool*, -limit* queries (Carl Dong)
6c5c60c4124293d948735756f84efc85262ea66f mempool: Use m_limit for UpdateTransactionsFromBlock (Carl Dong)
9e93b1030182eff92ef91181e17c7dd498c7e164 node/ifaces: Use existing MemPoolLimits (Carl Dong)
38af2bcf358a72b9457d370282e57f4be1c5c849 mempoolaccept: Use limits from mempool in constructor (Carl Dong)
9333427014695ac235c96d48791098168dfdc9db mempool: Introduce (still-unused) MemPoolLimits (Carl Dong)
716bb5fbd31077bbe99d11a54d6c2c250afc8085 scripted-diff: Rename anc/desc size limit vars to indicate SI unit (Carl Dong)
1ecc77321deb61b9f6888e4e10752b9d972fd26e scripted-diff: Rename DEFAULT_MEMPOOL_EXPIRY to indicate time unit (Carl Dong)
aa9141cd8185cb7ad532bc16feb9d302b05d9697 mempool: Pass in -mempoolexpiry instead of referencing gArgs (Carl Dong)
51c7a41a5eb6fcb60333812c770d80227cf7b64d init: Only determine maxmempool once (Carl Dong)
386c9472c8764738282e6d163b42e15a8feda7ea mempool: Make GetMinFee() with custom size protected (Carl Dong)
82f00de7a6a60cbc9ad0c6e1d0ffb1bc70c49af5 mempool: Pass in -maxmempool instead of referencing gArgs (Carl Dong)
f1941e8bfd2eecc478c7660434b1ebf6a64095a0 pool: Add and use MemPoolOptions, ApplyArgsManOptions (Carl Dong)
0199bd35bb44e32ee0db9b51c9d1bd7518c26f19 fuzz/rbf: Add missing TestingSetup (Carl Dong)
ccbaf546a68d6cda8ed3efd0598c0e4121b366bb scripted-diff: Rename DEFAULT_MAX_MEMPOOL_SIZE to indicate SI unit (Carl Dong)
fc02f77ca604f0221171bfde3059b34f5d0fb1cd ArgsMan: Add Get*Arg functions returning optional (Carl Dong)
Pull request description:
This is part of the `libbitcoinkernel` project: #24303, https://github.com/bitcoin/bitcoin/projects/18
-----
As mentioned in the Stage 1 Step 2 description of [the `libbitcoinkernel` project](https://github.com/bitcoin/bitcoin/issues/24303), `ArgsManager` will not be part of `libbitcoinkernel`. Therefore, it is important that we remove any dependence on `ArgsManager` by code that will be part of `libbitcoinkernel`. This is the first in a series of PRs aiming to achieve this.
This PR removes `CTxMemPool+MempoolAccept`'s dependency on `ArgsManager` by introducing a `CTxMemPool::Options` struct, which is used to specify `CTxMemPool`'s various options at construction time.
These options are:
- `-maxmempool` -> `CTxMemPool::Options::max_size`
- `-mempoolexpiry` -> `CTxMemPool::Options::expiry`
- `-limitancestorcount` -> `CTxMemPool::Options::limits::ancestor_count`
- `-limitancestorsize` -> `CTxMemPool::Options::limits::ancestor_size`
- `-limitdescendantcount` -> `CTxMemPool::Options::limits::descendant_count`
- `-limitdescendantsize` -> `CTxMemPool::Options::limits::descendant_size`
More context can be gleaned from the commit messages. The important commits are:
- 56eb479ded8bfb2ef635bb6f3b484f9d5952c70d "pool: Add and use MemPoolOptions, ApplyArgsManOptions"
- a1e08b70f3068f4e8def1c630d8f50cd54da7832 "mempool: Pass in -maxmempool instead of referencing gArgs"
- 6f4bf3ede5812b374828f08fc728ceded2f10024 "mempool: Pass in -mempoolexpiry instead of referencing gArgs"
- 5958a7fe4806599fc620ee8c1a881ca10fa2dd16 "mempool: Introduce (still-unused) MemPoolLimits"
Reviewers: Help needed in the following commits (see commit messages):
- a1e08b70f3068f4e8def1c630d8f50cd54da7832 "mempool: Pass in -maxmempool instead of referencing gArgs"
- 0695081a797e9a5d7787b78b0f8289dafcc6bff7 "node/ifaces: Use existing MemPoolLimits"
Note to Reviewers: There are perhaps an infinite number of ways to architect `CTxMemPool::Options`, the current one tries to keep it simple, usable, and flexible. I hope we don't spend too much time arguing over the design here since that's not the point. In the case that you're 100% certain that a different design is strictly better than this one in every regard, please show us a fully-implemented branch.
-----
TODO:
- [x] Use the more ergonomic `CTxMemPool::Options` where appropriate
- [x] Doxygen comments for `ApplyArgsManOptions`, `MemPoolOptions`
-----
Questions for Reviewers:
1. Should we use `std::chrono::seconds` for `CTxMemPool::Options::expiry` and `CTxMemPool::m_expiry` instead of an `int64_t`? Something else? (`std::chrono::hours`?)
2. Should I merge `CTxMemPool::Limits` inside `CTxMemPool::Options`?
ACKs for top commit:
MarcoFalke:
ACK d1684beabe5b738c2cc83de83e1aaef11a761b69 🍜
ryanofsky:
Code review ACK d1684beabe5b738c2cc83de83e1aaef11a761b69. Just minor cleanups since last review, mostly switching to brace initialization
Tree-SHA512: 2c138e52d69f61c263f1c3648f01c801338a8f576762c815f478ef5148b8b2f51e91ded5c1be915e678c0b14f6cfba894b82afec58d999d39a7bb7c914736e0b
|
|
Since:
- UpdateTransactionsFromBlock is only called by
MaybeUpdateMempoolForReorg, which calls it with the gArgs-determined
ancestor limits
- UpdateForDescendants is only called by UpdateTransactionsFromBlock
with the ancestor limits unchanged
We can remove the requirement to specify the ancestor limits for both
UpdateTransactionsFromBlock and UpdateForDescendants and just use the
values in the m_limits member.
Also move some removed comments to MemPoolLimits struct members.
The uint64_t cast in UpdateForDescendants is not new behavior,
see the diff in CChainState::MaybeUpdateMempoolForReorg for where they
were previously.
|
|
|
|
Better to be explicit when it comes to sizes to avoid unintentional
bugs. We use MB and KB all over the place.
-BEGIN VERIFY SCRIPT-
find_regex="DEFAULT_(ANCESTOR|DESCENDANT)_SIZE_LIMIT" \
&& git grep -l -E "$find_regex" \
| xargs sed -i -E "s@$find_regex@\0_KVB@g"
-END VERIFY SCRIPT-
|
|
- Store the mempool expiry (-mempoolexpiry) in CTxMemPool as a
std::chrono::seconds member.
- Remove the requirement to explicitly specify a mempool expiry for
LimitMempoolSize(...), just use the newly-introduced member.
- Remove all now-unnecessary instances of:
std::chrono::hours{gArgs.GetIntArg("-mempoolexpiry", DEFAULT_MEMPOOL_EXPIRY)}
|
|
- Store the mempool size limit (-maxmempool) in CTxMemPool as a member.
- Remove the requirement to explicitly specify a mempool size limit for
CTxMemPool::GetMinFee(...) and LimitMempoolSize(...), just use the
stored mempool size limit where possible.
- Remove all now-unnecessary instances of:
gArgs.GetIntArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE_MB) * 1000000
The code change in CChainState::GetCoinsCacheSizeState() is correct
since the coinscache should not repurpose "extra" mempool memory
headroom for itself if the mempool doesn't even exist.
|
|
Better to be explicit when it comes to sizes to avoid unintentional
bugs. We use MB and KB all over the place.
-BEGIN VERIFY SCRIPT-
find_regex="DEFAULT_MAX_MEMPOOL_SIZE" \
&& git grep -l -E "$find_regex" \
| xargs sed -i -E "s@$find_regex@\0_MB@g"
-END VERIFY SCRIPT-
|
|
|
|
|
|
-BEGIN VERIFY SCRIPT-
sed -i "s|node/ui_interface|node/interface_ui|g" $(git grep -l "node/ui_interface" ./src)
git mv src/node/ui_interface.cpp src/node/interface_ui.cpp
git mv src/node/ui_interface.h src/node/interface_ui.h
sed -i "s|BITCOIN_NODE_UI_INTERFACE_H|BITCOIN_NODE_INTERFACE_UI_H|g" src/node/interface_ui.h
-END VERIFY SCRIPT-
|
