Age | Commit message (Collapse) | Author |
|
|
|
These were uncovered as missing by the next commit.
|
|
This is an extraction of ArgsManager related functions from util/system
into their own common file.
Config file related functions are moved to common/config.cpp.
The background of this commit is an ongoing effort to decouple the
libbitcoinkernel library from the ArgsManager. The ArgsManager belongs
into the common library, since the kernel library should not depend on
it. See doc/design/libraries.md for more information on this rationale.
|
|
ApplyArgsManOptions does not need to set default values for missing
arguments, these are already defined in the BlockAssembler::Options.
This commit changes the interface of ApplyArgsManOptions(). If
ApplyArgsManOptions() is called again after a option is changed,
this option will no longer be reset to the default value.
There is no observed behaviour change due to how
ApplyArgsManOptions() is currently used, and the new interface is
consistent with e.g. ValidationCacheSizes and MemPoolLimits.
|
|
Add Options as a member to BlockAssembler to avoid having to assign
all the options individually.
Additionally brings the struct more in line with how we typically
define default and ArgManager values, as e.g. with
ChainstateManager::Options and and CTxMemPool::Options
|
|
Avoid confusion with the global gArgs
|
|
04528054fcde61aa00e009dbbe1ac350ca1cf748 [bench] BlockAssembler with mempool packages (glozow)
6ce265acf4ff6ee5057b46bcb8b55abc4422e6f8 [test util] lock cs_main before pool.cs in PopulateMempool (glozow)
8791410662ce3ab7ba6bbe9813c55369edd6e4c9 [test util] randomize fee in PopulateMempool (glozow)
cba5934eb697aedbe1966ebc2817ab87232a1b59 [miner] allow bypassing TestBlockValidity (glozow)
c0588523083c9c78770b8b19a52a919db56250d9 [refactor] parameterize BlockAssembler::Options in PrepareBlock (glozow)
a2de971ba1c588488dde653a76853666429d4911 [refactor] add helper to apply ArgsManager to BlockAssembler::Options (glozow)
Pull request description:
Performance of block template building matters as miners likely want to be able to start mining on a block with transactions asap after a block is found. We would want to know if a mempool PR accidentally caused, for example, a 100x slowdown. An `AssembleBlock()` bench exists, but it operates on a mempool with 101 transactions, each with 0 ancestors or descendants and with the same fee. Adding a bench with a more complex mempool is useful because (1) it's more realistic (2) updating packages can potentially cause the algorithm to take a long time.
ACKs for top commit:
kevkevinpal:
Tested ACK [0452805](https://github.com/bitcoin/bitcoin/pull/26695/commits/04528054fcde61aa00e009dbbe1ac350ca1cf748)
achow101:
ACK 04528054fcde61aa00e009dbbe1ac350ca1cf748
stickies-v:
ACK 04528054f
Tree-SHA512: 38c138d6a75616651f9b1faf4e3a1cd833437a486f4e84308fbee958e8462bb570582c88f7ba7ab99d80191e97855ac2cf27c43cc21585d3e4b0e227effe2fb5
|
|
ancestors
47c4b1f52ab8d95d7deef83050bad49d1e3e5990 mempool: log/halt when CalculateMemPoolAncestors fails unexpectedly (stickies-v)
5481f65849313ff947f38433b1ac28285a7f7694 mempool: add AssumeCalculateMemPoolAncestors helper function (stickies-v)
f911bdfff95eba3793fffaf71a31cc8bfc6f80c9 mempool: use util::Result for CalculateMemPoolAncestors (stickies-v)
66e028f7399b6511f9b73b1cef54b6a6ac38a024 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](https://github.com/bitcoin/bitcoin/blob/69b10212ea5370606c7a5aa500a70c36b4cbb58f/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 47c4b1f52ab8d95d7deef83050bad49d1e3e5990
w0xlt:
ACK https://github.com/bitcoin/bitcoin/pull/26289/commits/47c4b1f52ab8d95d7deef83050bad49d1e3e5990
glozow:
ACK 47c4b1f52ab8d95d7deef83050bad49d1e3e5990
furszy:
light code review ACK 47c4b1f5
aureleoules:
ACK 47c4b1f52ab8d95d7deef83050bad49d1e3e5990
Tree-SHA512: d908dad00d1a5645eb865c4877cc0bae74b9cd3332a3641eb4a285431aef119f9fc78172d38b55c592168a73dae83242e6af3348815f7b37cbe2d448a3a58648
|
|
-BEGIN VERIFY SCRIPT-
./contrib/devtools/copyright_header.py update ./
-END VERIFY SCRIPT-
Commits of previous years:
- 2021: f47dda2c58b5d8d623e0e7ff4e74bc352dfa83d7
- 2020: fa0074e2d82928016a43ca408717154a1c70a4db
- 2019: aaaaad6ac95b402fe18d019d67897ced6b316ee0
|
|
Allows us to test BlockAssembler on transactions without signatures or
mature coinbases (which is what PopulateMempool creates). Also means
that `TestBlockValidity()` is not included in the bench timing.
|
|
This allows us to both manually manipulate options and grab values from
ArgsManager (i.e. -blockmaxweight and -blockmintxfee config options)
when constructing BlockAssembler::Options. Prior to this change, the
only way to apply the config options is by ctoring BlockAssembler with
no options, which calls DefaultOptions().
|
|
When CalculateMemPoolAncestors fails unexpectedly (e.g. it exceeds
ancestor/descendant limits even though we expect no limits to be applied),
add an error log entry for increased visibility. For debug builds,
the application will even halt completely since this is not supposed
to happen.
|
|
Avoid using setAncestors outparameter, simplify function signatures
and avoid creating unused dummy strings.
|
|
fabf1cdb206e368a9433abf99a5ea2762a5ed2c0 Use steady clock for bench logging (MacroFake)
faed342a2338d6a1a26cf977671a736662debae4 scripted-diff: Rename time symbols (MacroFake)
Pull request description:
Instead of using `0.001` and similar constants to "convert" an int64_t to milliseconds, use the type-safe `Ticks<>` helper. Also, use steady clock instead of system clock, since the durations are used for benchmarking.
ACKs for top commit:
fanquake:
ACK fabf1cdb206e368a9433abf99a5ea2762a5ed2c0 - validation bench output still looks sane.
Tree-SHA512: e6525b5fdad6045ca500c56014897d7428ad288aaf375933d3b5939feddf257f6910d562eb66ebcde9186bef9a604ee8d763a318253838318d59df2a285be7c2
|
|
33b12e5df6aa14344dd084e0c6f2d34158fca383 docs: improve docs where MemPoolLimits is used (stickies-v)
6945853c0bf3b1941bfd18b5ffbd5a81be0e56da test: use NoLimits() in MempoolIndexingTest (stickies-v)
3a86f24a4c1e4e985b1d90eddc135b8dd17049a4 refactor: mempool: use CTxMempool::Limits (stickies-v)
b85af25f8770974bae4ef3fae64e75ef6dd2d3c2 refactor: mempool: add MemPoolLimits::NoLimits() (stickies-v)
Pull request description:
Mempool currently considers 4 limits regarding ancestor and descendant count and size, which get passed around between functions quite a bit. This PR uses `CTxMemPool::Limits` introduced in https://github.com/bitcoin/bitcoin/pull/25290 to simplify those signatures and callsites.
The purpose of this PR is to improve readability and maintenance, without behaviour change.
As noted in the first commit "refactor: mempool: change MemPoolLimits members to uint", we currently have an underflow issue where a user could pass a negative `-limitancestorsize`, which is eventually cast to an unsigned integer. This behaviour already exists. Because it's orthogonal and to minimize scope, I think this should be fixed in a separate PR.
ACKs for top commit:
hebasto:
ACK 33b12e5df6aa14344dd084e0c6f2d34158fca383, I have reviewed the code and it looks OK, I agree it can be merged.
glozow:
reACK 33b12e5df6aa14344dd084e0c6f2d34158fca383
Tree-SHA512: 591c6dcee1894f1c3ca28b34a680eeadcf0d40cda92451b4a422c03087b27d682b5e30ba4367abd75a99b5ccb115b7884b0026958d3c7dddab030549db5a4056
|
|
0f40d653218789aa176ca2f844e3222d2ad890a3 refactor: remove duplicate code from BlockAssembler (James O'Beirne)
Pull request description:
Found while reminding myself how transactions are chosen for blocks. Take it or leave it!
ACKs for top commit:
glozow:
ACK 0f40d653218789aa176ca2f844e3222d2ad890a3
theStack:
Concept and code-review ACK 0f40d653218789aa176ca2f844e3222d2ad890a3
Tree-SHA512: 8a2694e670ce3fe897ab8f64f64c8df5f8487fc1264527a3abbcba0e5b921fb693416497ccd62508295bc33f202c65556b91b6af463acb91aab43138d2492c14
|
|
Simplifies function signatures by removing repetition of all the
ancestor/descendant limits, and increases readability by being
more verbose by naming the limits, while still reducing the LoC.
|
|
|
|
-BEGIN VERIFY SCRIPT-
ren() { sed -i "s:\<$1\>:$2:g" $(git grep -l "\<$1\>" ':(exclude)src/versionbits.cpp') ; }
ren nStart time_start
ren nTimeStart time_start
ren nTimeReadFromDiskTotal time_read_from_disk_total
ren nTimeConnectTotal time_connect_total
ren nTimeFlush time_flush
ren nTimeChainState time_chainstate
ren nTimePostConnect time_post_connect
ren nTimeCheck time_check
ren nTimeForks time_forks
ren nTimeConnect time_connect
ren nTimeVerify time_verify
ren nTimeUndo time_undo
ren nTimeIndex time_index
ren nTimeTotal time_total
ren nTime1 time_1
ren nTime2 time_2
ren nTime3 time_3
ren nTime4 time_4
ren nTime5 time_5
ren nTime6 time_6
ren nBlocksTotal num_blocks_total
# Newline after semicolon
perl -0777 -pi -e 's/; time_connect_total/;\n time_connect_total/g' src/validation.cpp
perl -0777 -pi -e 's/; time_/;\n time_/g' src/validation.cpp
-END VERIFY SCRIPT-
|
|
-BEGIN VERIFY SCRIPT-
sed -i 's/CChainState/Chainstate/g' $(git grep -l CChainState ':(exclude)doc/release-notes*')
-END VERIFY SCRIPT-
Co-authored-by: MacroFake <falke.marco@gmail.com>
|
|
|
|
...also adjust callers
Changes:
- In BlockAssembler::CreateNewBlock, we now only lock m_mempool->cs and
call addPackageTxs if m_mempool is not nullptr
- BlockAssembler::addPackageTxs now takes in a mempool reference, and is
annotated to require that mempool's lock.
- In TestChain100Setup::CreateBlock and generateblock, don't construct
an empty mempool, just pass in a nullptr for mempool
|
|
Since UpdatePackagesForAdded is a helper function that's only used in
addPackageTxs we can make it static and avoid the unnecessary interface
and in-header lock annotation.
|
|
SkipMapTxEntry is a short helper function that's only used in
addPackageTxs, we can just inline it, keep the comments, and avoid the
unnecessary interface and lock annotations.
|
|
UpdatePackagesForAdded at beginning of addPackageTxs
7036cf52aa080d2a63993c2298555252d507dd2f Delete UpdatePackagesForAdded at beginning of addPackageTxs. (KevinMusgrave)
Pull request description:
In `CreateNewBlock` (in miner.cpp), `inBlock` is cleared before `addPackageTxs`, so `inBlock` will be empty in the first call to `UpdatePackagesForAdded`. I saw this brought up in these [PR review club logs](https://bitcoincore.reviews/24538) and there didn't seem to be a definitive answer for why the call is necessary. There's also an [old PR](https://github.com/bitcoin/bitcoin/pull/10200) where this change was going to be applied, but it got closed.
If `addPackageTxs` can be called when `inBlock` is not empty, then maybe a test should be added for that case. All the tests seem to pass with this deletion.
ACKs for top commit:
glozow:
utACK 7036cf52aa080d2a63993c2298555252d507dd2f
Tree-SHA512: 9e757b71b9035f68a0c6fef229b8cd83f1bdbe23f05bb02cc1bab8c3c177805b388bceb2bb1f0bce354791ccb29f351a6c51979b96ffe4d9fc6c978f83e36afc
|
|
This decouples validation.cpp from netaddress.cpp (transitively,
timedata.cpp, and asmap.cpp).
This is important for libbitcoinkernel as:
- There is no reason for the consensus engine to be coupled with
netaddress, timedata, and asmap
- Users of libbitcoinkernel can now easily supply their own
std::function that provides the adjusted time.
See the src/Makefile.am changes for some satisfying removals.
|
|
|
|
|
|
GenerateCoinbaseCommitment into ChainstateManager
|
|
As described in commit 9cea7e37158aa85119de2c81e93901da9e476ee5, this is no longer needed because priority transaction selection (addPriorityTxs) was removed in commit 272b25a6a99057fdcd5db5bce70b49625e973080.
|
|
|
|
f865cf8ded2b2fbc82a6fbc41226d991909a6880 Add and use BlockManager::GetAllBlockIndices (Carl Dong)
28ba0313eac37e4a900b7e97af7169ce999c4024 Add and use CBlockIndexHeightOnlyComparator (Carl Dong)
12eb05df63f930969115af6dc66e2e5d02f2a517 move-only: Move CBlockIndexWorkComparator to blockstorage (Carl Dong)
c600ee38168a460d3026eae0e289c976194aad14 Only load BlockMan in BlockMan member functions (Carl Dong)
42e56d9b188f97c077ed2269e24acc0be35ece17 style-only: No need for std::pair for vSortedByHeight (Carl Dong)
3bbb6fea051f4e19bd2448e401a6c4e9b4cc7a41 style-only: Various blockstorage.cpp cleanups (Carl Dong)
5be9ee3c54dcb396ff52fc8c8b7e4e6e39ec4a3b refactor: more const annotations for uses of CBlockIndex* (Anthony Towns)
Pull request description:
The only important commit is "Only load BlockMan in BlockMan member functions", everything else is all just small style changes.
Here's the commit message, reproduced:
```
This commit effectively splits the "load block index itself" logic from
"derive Chainstate variables from loaded block index" logic.
This means that BlockManager::LoadBlockIndex{,DB} will only load what's
relevant to the BlockManager.
```
ACKs for top commit:
ajtowns:
ACK f865cf8ded2b2fbc82a6fbc41226d991909a6880 ; code review only
MarcoFalke:
review ACK f865cf8ded2b2fbc82a6fbc41226d991909a6880 🗂
Tree-SHA512: 7b204d782834e06fd7329d022e2ae860181b4e8105c33bfb928539a4ec24161dc7438a9c4d4ee279dcad77de310c160b997bb8aa18923243d0fd55ccf4ad7c3a
|
|
|
|
Given the low possibility of a reorg reverting the segwit soft fork,
there is no need to check whether segwit is active here. Also,
TestBlockValidity is run on the block template after it has been
created.
|
|
|
|
|
|
-BEGIN VERIFY SCRIPT-
./contrib/devtools/copyright_header.py update ./
-END VERIFY SCRIPT-
Commits of previous years:
* 2020: fa0074e2d82928016a43ca408717154a1c70a4db
* 2019: aaaaad6ac95b402fe18d019d67897ced6b316ee0
|
|
in src/node/miner to:
- BlockAssembler::addPackageTxs()
- BlockAssembler::SkipMapTxEntry()
- BlockAssembler::UpdatePackagesForAdded()
These functions have thread safety lock annotations in
their declarations but are missing the corresponding
run-time lock assertions in their definitions.
Per doc/developer-notes.md: "Combine annotations in function
declarations with run-time asserts in function definitions."
|
|
|
|
Can be reviewed with --word-diff-regex=. --ignore-all-space
|
|
|
|
-BEGIN VERIFY SCRIPT-
# Move module
git mv src/miner.cpp src/node/
git mv src/miner.h src/node/
# Replacements
sed -i 's:miner\.h:node/miner.h:g' $(git grep -l miner)
sed -i 's:miner\.cpp:node/miner.cpp:g' $(git grep -l miner)
sed -i 's:MINER_H:NODE_MINER_H:g' $(git grep -l MINER_H)
-END VERIFY SCRIPT-
|