Age | Commit message (Collapse) | Author |
|
|
|
Apart from tests, it is only used in one place, so there is no need for
an alias.
|
|
|
|
|
|
`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
|
|
|
|
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 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 -e "s/static const /static constexpr /" src/policy/policy.h
-END VERIFY SCRIPT-
|
|
|
|
|
|
|
|
|
|
Also fix includes using iwyu
|
|
|
|
This comment described how the constructor of CFeeRate was previously
indirectly used to parse fee rate arguments from RPCs. The command line
input was actually in sat/vB but due to the use of AmountFromValue() it
got converted to BTC/vB. In the constructor this could be rectified by
creating a CFeeRate from that given value (in BTC/vB) and COIN as the
transaction size, turning the input effectively to sat/vB. Since this
usage pattern was removed from the codebase some months ago, the comment
is now obsolete.
Also:
• Use vsize and vbyte instead of size and byte
|
|
Behavior change: don't quit right after LimitMempoolSize() when a
package is partially submitted. We should still send
TransactionAddedToMempool notifications for
transactions that were submitted.
Not behavior change: add a new package validation result for mempool logic errors.
|
|
-BEGIN VERIFY SCRIPT-
./contrib/devtools/copyright_header.py update ./
-END VERIFY SCRIPT-
Commits of previous years:
* 2020: fa0074e2d82928016a43ca408717154a1c70a4db
* 2019: aaaaad6ac95b402fe18d019d67897ced6b316ee0
|
|
for packages of 1 child + parents
046e8ff264be6b888c0f9a9d822e32aa74e19b78 [unit test] package submission (glozow)
e12fafda2dfbbdf63f125e5af797ecfaa6488f66 [validation] de-duplicate package transactions already in mempool (glozow)
8310d942e046c5a9b6bd90afdcd3af68dd91e081 [packages] add sanity checks for package vs mempool limits (glozow)
be3ff151a1f9665720cdf70d072b098a2f9726a9 [validation] full package accept + mempool submission (glozow)
144a29099a865ac1dc3e5291d9529fbcca9c83a4 [policy] require submitted packages to be child-with-unconfirmed-parents (glozow)
d59ddc5c3d1c035474d7bc9fa9f8a0eeb1c8498c [packages/doc] define and document package rules (glozow)
ba26169f6035c238378a3c9647213328a006fa23 [unit test] context-free package checks (glozow)
9b2fdca7f03911ac40fe0f8a0b5da534bee4554b [packages] add static IsChildWithParents function (glozow)
Pull request description:
This is 1 chunk of [Package Mempool Accept](https://gist.github.com/glozow/dc4e9d5c5b14ade7cdfac40f43adb18a); it restricts packages to 1 child with its parents, doesn't allow conflicts, and doesn't have CPFP (yet). Future PRs (see #22290) will add RBF and CPFP within packages.
ACKs for top commit:
laanwj:
Code review ACK 046e8ff264be6b888c0f9a9d822e32aa74e19b78
Tree-SHA512: 37dbba37d527712f8efef71ee05c90a8308992615af35f5e0cfeafc60d859cc792737d125aac526e37742fe7683ac8c155ac24af562426213904333c01260c95
|
|
11daf6ceb1d9ea1f8d638b123eecfe39d162a7c3 More Span simplifications (Pieter Wuille)
568dd2f83900a11a4dbba1250722791a135bf0a9 Replace MakeSpan helper with Span deduction guide (Pieter Wuille)
Pull request description:
C++17 supports [user-defined deduction guides](https://en.cppreference.com/w/cpp/language/class_template_argument_deduction), allowing class constructors to be invoked without specifying class template arguments. Instead, the code can contain rules to infer the template arguments from the constructor argument types.
This alleviates the need for the `MakeSpan` helper. Convert the existing MakeSpan rules into deduction rules for `Span` itself, and replace all invocations of `MakeSpan` with just `Span` ones.
ACKs for top commit:
MarcoFalke:
re-ACK 11daf6ceb1d9ea1f8d638b123eecfe39d162a7c3 Only change is removing a hunk in the tests 🌕
Tree-SHA512: 10f3e82e4338f39d9b7b407cd11aac7ebe1e9191b58e3d7f4e5e338a4636c0e126b4a1d912127c7446f57ba356c8d6544482e47f97901efea6a54fffbfd7895f
|
|
8c277b19c8f262e550cffe263e6d910b687ac882 refactor: Make m_cs_fee_estimator non-recursive (Hennadii Stepanov)
5ee5b696b588695ff78aaac08d5d85154f1953cf refactor: Add non-thread-safe CBlockPolicyEstimator::_removeTx helper (Hennadii Stepanov)
5c3033d45e5ec15499ce7a0222ffa0210a0f66bc Add thread safety annotations to CBlockPolicyEstimator public functions (Hennadii Stepanov)
Pull request description:
This PR eliminates the only place that `m_cs_fee_estimator` is recursively locked by refactoring out `_removeTx` member function.
Related to #19303.
ACKs for top commit:
theStack:
Code-review ACK 8c277b19c8f262e550cffe263e6d910b687ac882
amadeuszpawlik:
ACK 8c277b19c8f262e550cffe263e6d910b687ac882 reviewed, built and ran tests
Tree-SHA512: 65b0b59460d3d5fadf7e75e916b2898b0dcfafdf5b278ef8c3975660f67c9f88ae4b937944313bd36d7513a7a53e1e5859aaf4a6deb4a1aea089936b101635a1
|
|
|
|
|
|
fa3e0da06b491b8c0fa2dbae37682a9112c9deb8 policy: Treat taproot as always active (MarcoFalke)
Pull request description:
Now that taproot is active, it can be treated as if it was always active for policy for the next major release. This simplifies the code and changes two things:
* Importing `tr` descriptors can be done before the chain is fully synced. This is fine, because the wallet will already generate `tr` descriptors by default (regardless of the taproot status) after commit 47fe7445e7f54aee10ec6dbc53f1db1adbeb43de.
* Valid taproot spends won't be rejected from the mempool before taproot is active. This is strictly speaking a bugfix after commit 47fe7445e7f54aee10ec6dbc53f1db1adbeb43de, since the wallet may generate taproot spends before the chain is fully synced. For example, a slow node or a purposefully offline node. Currently, the wallet needs the mempool to account for change. See https://github.com/bitcoin/bitcoin/issues/11887.
A similar change was done for segwit v0 in https://github.com/bitcoin/bitcoin/pull/13120 .
This effectively reverts commit c5ec0367d718544caa3a1578d6c730fc92ee4e94.
ACKs for top commit:
mjdietzx:
Code Review ACK fa3e0da06b491b8c0fa2dbae37682a9112c9deb8
achow101:
ACK fa3e0da06b491b8c0fa2dbae37682a9112c9deb8
sipa:
utACK fa3e0da06b491b8c0fa2dbae37682a9112c9deb8
gruve-p:
ACK https://github.com/bitcoin/bitcoin/pull/23512/commits/fa3e0da06b491b8c0fa2dbae37682a9112c9deb8
gunar:
Code Review + tACK fa3e0da06
rajarshimaitra:
code review + tACK https://github.com/bitcoin/bitcoin/pull/23512/commits/fa3e0da06b491b8c0fa2dbae37682a9112c9deb8
Tree-SHA512: c6dc7a4e6c345bdec33f256847dc63906ab1696aa683ab9b32a79e715613950884ac3a1a7a44e95f31bb28e58dd64679a616175f7e152b21f5550f3337c8e622
|
|
|
|
|
|
than expected feerate
80dc829be7f8c3914074b85bb4c125baba18cb2c tests: Calculate fees more similarly to CFeeRate::GetFee (Andrew Chow)
ce2cc44afd51f3df4ee7f14ea05b8da229183923 tests: Test for assertion when feerate is rounded down (Andrew Chow)
0fbaef9676a1dcb84bcf95afd8d994831ab327b6 fees: Always round up fee calculated from a feerate (Andrew Chow)
Pull request description:
When calculating the fee for a feerate, it is possible that the final calculation will have fractional satoshis. Currently those are ignored via truncation which results in the absolute fee being rounded down. Rounding down is problematic because it results in a feerate that is slightly lower than the feerate represented by the `CFeeRate` object. A slightly lower feerate particularly causes issues for coin selection as it can trigger an assertion error. To avoid potentially underpaying the feerate (and the assertion), always round up the calculated fee.
A test is added for the assertion, along with a comment explaining what happens.
It is unlikely that a user can trigger this as it requires a very specific set of rounding errors to occur as well as the transaction not needing any change and being right on the lower bound of the exact match window. However I was able to trigger the assertion while running coin selection simulations, albeit after thousands of transactions and with some weird feerates.
ACKs for top commit:
ryanofsky:
Code review ACK 80dc829be7f8c3914074b85bb4c125baba18cb2c
promag:
Tested ACK 80dc829be7f8c3914074b85bb4c125baba18cb2c.
lsilva01:
tACK 80dc829
meshcollider:
utACK 80dc829be7f8c3914074b85bb4c125baba18cb2c
Tree-SHA512: fe26684c60f236cab48ea6a4600c141ce766dbe59504ec77595dcbd7fd0b34559acc617007f4f499c9155d8fda0a336954413410ba862b19c765c0cfac79d642
|
|
Allowing callers to pass in a uint256 (which could be txid or wtxid)
but then always assuming that it's a txid is a footgunny interface.
|
|
::ChainActive()
a0efe529e4fd053b890450413b9ca5e1bcd8f2c2 Fix outdated comments referring to ::ChainActive() (Samuel Dobson)
Pull request description:
After #21866 there are a few outdated comments referring to `::ChainActive()`, which should instead refer to `ChainstateManager::ActiveChain()`.
ACKs for top commit:
jamesob:
ACK https://github.com/bitcoin/bitcoin/pull/23258/commits/a0efe529e4fd053b890450413b9ca5e1bcd8f2c2
Tree-SHA512: 80da19c105ed29ac247e6df4c8e916c3bf3f37230b63f07302114eef9c115add673e9649f0bbe237295be0c6da7b1030b5b93e14daf6768f17ce5de7cf2c9ff2
|
|
d873db7f8ff37c37f9c72482d8ecd52282f4438e policy: document we intentionally don't lower the dust threshold for Taproot (Antoine Poinsot)
Pull request description:
Following discussions in #22779 .
ACKs for top commit:
benthecarman:
ACK d873db7f8ff37c37f9c72482d8ecd52282f4438e
ariard:
Code Review ACK d873db7
theStack:
ACK d873db7f8ff37c37f9c72482d8ecd52282f4438e
Tree-SHA512: 1f5d20dce767f8a74d57ece47a7f6b881741f508896131b8433600cccf9e4262892603b46521d1bb69d5c83b450f24a16731341072a471c1f2c9adad682af895
|
|
|
|
When calculating the fee for a given tx size from a fee rate, we should
always round up to the next satoshi. Otherwise, if we round down (via
truncation), the calculated fee may result in a fee with a feerate
slightly less than targeted.
This is particularly important for coin selection as a slightly lower
feerate than expected can result in a variety of issues.
|
|
There is no change in behavior. This just helps prepare for the
transition from boost::filesystem to std::filesystem by avoiding calls
to methods which will be unsafe after the transaction to std::filesystem
to due lack of a boost::filesystem::path::imbue equivalent and inability
to set a predictable locale.
Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
Co-authored-by: Kiminuo <kiminuo@protonmail.com>
Co-authored-by: MarcoFalke <falke.marco@gmail.com>
|
|
Move amount.h to consensus/amount.h.
Renames, adds missing and removes uneeded includes.
|
|
|
|
Document a few non-obvious things and delete no-longer-relevant comments
(e.g. about taking a lock that we're already holding).
No change in behavior.
|
|
|
|
|
|
"Fee Delta" is already a term used for prioritizing transactions:
modified = base fees + delta
Here, delta also means the difference between original and modified replacement fees:
nDeltaFees = (original_base + original_delta) - (replacement_base + replacement_delta)
This is insanely confusing. Also, since mempool is no longer a member of a
class (MemPoolAccept.m_pool), the "m" prefix is unnecessary. The rest are
clarity/style-focused changes to already-touched lines.
-BEGIN VERIFY SCRIPT-
ren() { sed -i "s/\<$1\>/$2/g" src/policy/rbf* ; }
ren nDeltaFees additional_fees
ren m_pool pool
ren nSize replacement_vsize
ren nModifiedFees replacement_fees
ren nConflictingFees original_fees
ren oldFeeRate original_feerate
ren newFeeRate replacement_feerate
ren setAncestors ancestors
ren setIterConflicting iters_conflicting
ren setConflictsParents parents_of_conflicts
ren setConflicts direct_conflicts
ren allConflicting all_conflicts
sed -i "s/ hash\b/ txid/g" src/policy/rbf*
-END VERIFY SCRIPT-
|
|
|
|
|
|
This checks that a transaction isn't trying to replace something it
supposedly depends on.
|
|
|
|
Avoids reusing err_string.
|
|
A "correction" of what seemed to be an overlook was initially proposed in
PR #22779. It was deemed unnecessary to further reduce the dust level,
so document the intention.
Signed-off-by: Antoine Poinsot <darosior@protonmail.com>
|