Since the kernel library no longer depends on the system file, move it
to the common library instead in accordance to the diagram in
doc/design/libraries.md.
47c4b1f52a mempool: log/halt when CalculateMemPoolAncestors fails unexpectedly (stickies-v)
5481f65849 mempool: add AssumeCalculateMemPoolAncestors helper function (stickies-v)
f911bdfff9 mempool: use util::Result for CalculateMemPoolAncestors (stickies-v)
66e028f739 mempool: use util::Result for CalculateAncestorsAndCheckLimits (stickies-v)
Pull request description:
Upon reviewing the documentation for `CTxMemPool::CalculateMemPoolAncestors`, I noticed `setAncestors` was meant to be an `out` parameter but actually is an `in,out` parameter, as can be observed by adding `assert(setAncestors.empty());` as the first line in the function and running `make check`. This PR fixes this unexpected behaviour and introduces refactoring improvements to make intents and effects of the code more clear.
## Unexpected behaviour
This behaviour occurs only in the package acceptance path, currently only triggered by `testmempoolaccept` and `submitpackage` RPCs.
In `MemPoolAccept::AcceptMultipleTransactions()`, we first call `PreChecks()` and then `SubmitPackage()` with the same `Workspace ws` reference. `PreChecks` leaves `ws.m_ancestors` in a potentially non-empty state, before it is passed on to `MemPoolAccept::SubmitPackage`. `SubmitPackage` is the only place where `setAncestors` isn't guaranteed to be empty before calling `CalculateMemPoolAncestors`. The most straightforward fix is to just forcefully clear `setAncestors` at the beginning of CalculateMemPoolAncestors, which is done in the first bugfix commit.
## Improvements
### Return value instead of out-parameters
This PR updates the function signatures for `CTxMemPool::CalculateMemPoolAncestors` and `CTxMemPool::CalculateAncestorsAndCheckLimits` to use a `util::Result` return type and eliminate both the `setAncestors` `in,out`-parameter as well as the error string. It simplifies the code and makes the intent and effects more explicit.
### Observability
There are 7 instances where we currently call `CalculateMemPoolAncestors` without actually checking if the function succeeded because we assume that it can't fail, such as in [miner.cpp](69b10212ea/src/node/miner.cpp (L399)). This PR adds a new wrapper `AssumeCalculateMemPoolAncestors` function that logs such unexpected failures, or in case of debug builds even halts the program. It's not crucial to the objective, more of an observability improvement that seems sensible to add on here.
ACKs for top commit:
achow101:
ACK 47c4b1f52a
w0xlt:
ACK 47c4b1f52a
glozow:
ACK 47c4b1f52a
furszy:
light code review ACK 47c4b1f5
aureleoules:
ACK 47c4b1f52a
Tree-SHA512: d908dad00d1a5645eb865c4877cc0bae74b9cd3332a3641eb4a285431aef119f9fc78172d38b55c592168a73dae83242e6af3348815f7b37cbe2d448a3a58648
The (100, 1000000, 1000, 1000000) limits are arbitrarily high and
don't restrict anything, they are just meant to calculate ancestors
properly. Using NoLimits() makes this intent more clear and simplifies
the code.
The version of GetMinFee() with a custom size specification is and
should only be used by tests. Mark it as protected and use a derived
class exposing GetMinFee() as public in tests.
After this commit, there should be no explicit instantiation of
CTxMemPool in src/test other than those in fuzz/ and setup_common
-BEGIN VERIFY SCRIPT-
find_regex="CTxMemPool\s+([^;({]+)(|\(\)|\{\});" \
&& git grep -l -E "$find_regex" -- src/test \
| grep -v -e "^src/test/util/setup_common.cpp$" \
-e "^src/test/fuzz/" \
| xargs sed -i -E "s@$find_regex@CTxMemPool\& \1 = *Assert(m_node.mempool);@g"
-END VERIFY SCRIPT-
fa587773e5 scripted-diff: Remove unused first argument to addUnchecked (MarcoFalke)
fe5c49766c tx pool: Use the entry's hash instead of the one passed to addUnchecked (MarcoFalke)
ddd395f968 Mark CTxMemPoolEntry members that should not be modified const (MarcoFalke)
Pull request description:
Several years ago the transaction hash was not cached. For optimization the hash was instead passed into `addUnchecked` to avoid re-calculating it. See f77654a0e9
Passing in the hash is now redundant and the argument can safely be removed.
Tree-SHA512: 0206b65c7a014295f67574120e8c5397bf1b1bd70c918ae1360ab093676f7f89a6f084fd2c7000a141baebfe63fe6f515559e38c4ac71810ba64f949f9c0467f
3fc20632a3 qt: Set BLOCK_CHAIN_SIZE = 220 (DrahtBot)
2b6a2f4a28 Regenerate manpages (DrahtBot)
eb7daf4d60 Update copyright headers to 2018 (DrahtBot)
Pull request description:
Some trivial maintenance to avoid having to do it again after the 0.17 branch off.
(The scripts to do this are in `./contrib/`)
Tree-SHA512: 16b2af45e0351b1c691c5311d48025dc6828079e98c2aa2e600dc5910ee8aa01858ca6c356538150dc46fe14c8819ed8ec8e4ec9a0f682b9950dd41bc50518fa
0a22a52 Use mempool's ancestor sort in transaction selection (Suhas Daftuar)
7abfa53 Add test for new ancestor feerate sort behavior (Suhas Daftuar)
9a51319 Sort mempool by min(feerate, ancestor_feerate) (Suhas Daftuar)
6773f92 Refactor CompareTxMemPoolEntryByDescendantScore (Suhas Daftuar)
Pull request description:
This more closely approximates the desirability of a given transaction for
mining, and should result in less re-sorting when transactions get removed from
the mempool after being mined.
I measured this as approximately a 5% speedup in removeForBlock.
Tree-SHA512: ffa36b567c5dfe3e8908c545a459b6a5ec0de26e7dc81b1050dd235cac9046564b4409a3f8c5ba97bd8b30526e8fec8f78480a912e317979467f32305c3dd37b
This resolves an issue where estimatesmartfee would return 999
sat/byte instead of 1000, due to floating point loss of precision
Thanks to sipa for suggesting is_integral.
Fee estimation can just check its own mapMemPoolTxs to determine the same information. Note that now fee estimation for block processing must happen before those transactions are removed, but this shoudl be a speedup.
