Commit graph

282 commits

Author SHA1 Message Date
MacroFake
e4e201dfd9
Merge bitcoin/bitcoin#25290: [kernel 3a/n] Decouple CTxMemPool from ArgsManager
d1684beabe fees: Pass in a filepath instead of referencing gArgs (Carl Dong)
9a3d825c30 init: Remove redundant -*mempool*, -limit* queries (Carl Dong)
6c5c60c412 mempool: Use m_limit for UpdateTransactionsFromBlock (Carl Dong)
9e93b10301 node/ifaces: Use existing MemPoolLimits (Carl Dong)
38af2bcf35 mempoolaccept: Use limits from mempool in constructor (Carl Dong)
9333427014 mempool: Introduce (still-unused) MemPoolLimits (Carl Dong)
716bb5fbd3 scripted-diff: Rename anc/desc size limit vars to indicate SI unit (Carl Dong)
1ecc77321d scripted-diff: Rename DEFAULT_MEMPOOL_EXPIRY to indicate time unit (Carl Dong)
aa9141cd81 mempool: Pass in -mempoolexpiry instead of referencing gArgs (Carl Dong)
51c7a41a5e init: Only determine maxmempool once (Carl Dong)
386c9472c8 mempool: Make GetMinFee() with custom size protected (Carl Dong)
82f00de7a6 mempool: Pass in -maxmempool instead of referencing gArgs (Carl Dong)
f1941e8bfd pool: Add and use MemPoolOptions, ApplyArgsManOptions (Carl Dong)
0199bd35bb fuzz/rbf: Add missing TestingSetup (Carl Dong)
ccbaf546a6 scripted-diff: Rename DEFAULT_MAX_MEMPOOL_SIZE to indicate SI unit (Carl Dong)
fc02f77ca6 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 d1684beabe 🍜
  ryanofsky:
    Code review ACK d1684beabe. Just minor cleanups since last review, mostly switching to brace initialization

Tree-SHA512: 2c138e52d69f61c263f1c3648f01c801338a8f576762c815f478ef5148b8b2f51e91ded5c1be915e678c0b14f6cfba894b82afec58d999d39a7bb7c914736e0b
2022-06-29 09:13:31 +02:00
Carl Dong
6c5c60c412 mempool: Use m_limit for UpdateTransactionsFromBlock
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.
2022-06-28 15:49:15 -04:00
Carl Dong
9333427014 mempool: Introduce (still-unused) MemPoolLimits
They live as a CTxMemPool member.

[META] These limits will be used in subsequent commits to replace calls
       to gArgs.
2022-06-28 15:46:10 -04:00
Carl Dong
aa9141cd81 mempool: Pass in -mempoolexpiry instead of referencing gArgs
- 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)}
2022-06-28 15:42:23 -04:00
Carl Dong
386c9472c8 mempool: Make GetMinFee() with custom size protected
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.
2022-06-28 15:36:36 -04:00
Carl Dong
82f00de7a6 mempool: Pass in -maxmempool instead of referencing gArgs
- 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.
2022-06-28 15:36:18 -04:00
Carl Dong
f1941e8bfd pool: Add and use MemPoolOptions, ApplyArgsManOptions
Reviewers: Note that CTxMemPool now requires a non-defaulted
CTxMemPool::Options for its constructor. Meaning that there's no need to
worry about a stray CTxMemPool constructor somewhere defaulting to
something incorrect. All instances of CTxMemPool construction are
addressed here in this commit.

We set options for CTxMemPool and construct it in many different ways. A
good example can be seen in how we determine CTxMemPool's check_ratio in
AppInitMain(...).

1. We first set the default based on chainparams's
   DefaultConsistencyChecks()
2. Then, we apply the ArgsManager option on top of that default
3. Finally, we clamp the result of that between 0 and 1 Million

With this patch, most CTxMemPool construction are along the lines of:

    MemPoolOptions mempool_opts{...default overrides...};
    ApplyArgsManOptions(argsman, mempool_opts);
    ...hard overrides...
    CTxMemPool pool{mempool_opts};

This "compositional" style of building options means that we can omit
unnecessary/irrelevant steps wherever we want but also maintain full
customizability.

For example:

- For users of libbitcoinkernel, where we eventually want to remove
  ArgsManager, they simply won't call (or even know about)
  ApplyArgsManOptions.

- See src/init.cpp to see how the check_ratio CTxMemPool option works
  after this change.

A MemPoolOptionsForTest helper was also added and used by tests/fuzz
tests where a local CTxMemPool needed to be created.

The change in src/test/fuzz/tx_pool.cpp seemingly changes behaviour by
applying ArgsManager options on top of the CTxMemPool::Options defaults.
However, in future commits where we introduce flags like -maxmempool,
the call to ApplyArgsManOptions is actually what preserves the existing
behaviour. Previously, although it wasn't obvious, our CTxMemPool would
consult gArgs for flags like -maxmempool when it needed it, so it
already relied on ArgsManager information. This patchset just laid bare
the obfuscatory perils of globals.

[META] As this patchset progresses, we will move more and more
       CTxMemPool-relevant options into MemPoolOptions and add their
       ArgsMan-related logic to ApplyArgsManOptions.
2022-06-28 15:30:05 -04:00
MarcoFalke
fa52cf8e11
refactor: Replace feeDelta by m_modified_fee
* feeDelta tracked the delta (to be applied on top of the actual fee)
* m_modified_fee tracks the actual fee with the delta included
* Instead of passing in the new total delta to the Updater, pass in by
  how much the total delta should be modified.

This is needed for the next commit, but makes sense on its own because
the same is done by UpdateDescendantState and UpdateAncestorState.
2022-06-22 09:32:07 +02:00
MarcoFalke
fa84a49526
Use CAmount for fee delta and modified fee 2022-03-21 13:38:08 +01:00
fanquake
0147278e37
Merge bitcoin/bitcoin#21464: Mempool Update Cut-Through Optimization
c5b36b1c1b Mempool Update Cut-Through Optimization (Jeremy Rubin)
c49daf9885 [TESTS] Increase limitancestorcount in tournament RPC test to showcase improved algorithm (Jeremy Rubin)

Pull request description:

  Often when we're updating mempool entries we update entries that we ultimately end up removing the updated entries shortly thereafter. This patch makes it so that we filter for such entries a bit earlier in processing, which yields a mild improvement for these cases, and is negligible overhead otherwise.

  There's potential for a better -- but more sophisticated -- algorithm that can be used taking advantage of epochs, but I figured it is better to do something that is simple and works first and upgrade it later as the other epoch mempool work proceeds as it makes the patches for the epoch algorithm simpler to understand, so you can consider this as preparatory work. It could either go in now if it is not controversial, or we could wait until the other patch is ready to go.

ACKs for top commit:
  instagibbs:
    reACK c5b36b1
  sipa:
    utACK c5b36b1c1b
  mzumsande:
    Code Review ACK c5b36b1c1b

Tree-SHA512: 78b16864f77a637d8a68a65e23c019a9757d8b2243486728ef601d212ae482f6084cf8e69d810958c356f1803178046e4697207ba40d6d10529ca57de647fae6
2022-01-25 11:20:18 +08:00
glozow
e177fcab38 Replace struct update_lock_points with lambda
No behavior change.
This code was introduced in 5add7a7 before we required C++11, which is
why the struct was needed. As we are now using more modern C++ and this
is the only place where lockpoints are updated for mempool entries, it
is more idiomatic to call `modify` with a lambda.

Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
2022-01-18 11:55:15 +00:00
glozow
c7cd98c717 document and clean up MaybeUpdateMempoolForReorg
Co-authored-by: John Newbery <john@johnnewbery.com>
2022-01-18 11:55:06 +00:00
MarcoFalke
75a227e39e
Merge bitcoin/bitcoin#23683: bug fix: valid but different LockPoints after a reorg
b4adc5ad67 [bugfix] update lockpoints correctly during reorg (glozow)
b6002b07a3 MOVEONLY: update_lock_points to txmempool.h (glozow)

Pull request description:

  I introduced a bug in #22677 (sorry! 😅)

  Mempool entries cache `LockPoints`, containing the first height/blockhash/`CBlockIndex*` at which the transaction becomes valid. During a reorg, we re-check timelocks on all mempool entries using `CheckSequenceLocks(useExistingLockPoints=false)` and remove any now-invalid entries. `CheckSequenceLocks()` also mutates the `LockPoints` passed in, and we update valid entries' `LockPoints` using `update_lock_points`. Thus, `update_lock_points(lp)` needs to be called right after `CheckSequenceLocks(lp)`, otherwise we lose the data in `lp`. I incorrectly assumed they could be called in separate loops.

  The incorrect behavior introduced is: if we have a reorg in which a timelocked mempool transaction is still valid but becomes valid at a different block, the cached `LockPoints` will be incorrect.

  This PR fixes the bug, adds a test, and adds an assertion at the end of `removeForReorg()` to check that all mempool entries' lockpoints are valid. You can reproduce the bug by running the test added in the [test] commit on the code before the [bugfix] commit.

ACKs for top commit:
  jnewbery:
    ACK b4adc5ad67
  vasild:
    ACK b4adc5ad67
  mzumsande:
    Code Review ACK b4adc5ad67
  hebasto:
    ACK b4adc5ad67
  MarcoFalke:
    re-ACK b4adc5ad67 🏁

Tree-SHA512: 16b59f6ff8140d0229079ca1c6b04f2f4a00a2e49931275150e4f3fe5ac4ec109698b083fa6b223ba9511f328271cc1ab081263669d5da020af7fee83c13e401
2022-01-03 14:34:39 +01:00
Hennadii Stepanov
f47dda2c58
scripted-diff: Bump copyright headers
-BEGIN VERIFY SCRIPT-
./contrib/devtools/copyright_header.py update ./
-END VERIFY SCRIPT-

Commits of previous years:
* 2020: fa0074e2d8
* 2019: aaaaad6ac9
2021-12-30 19:36:57 +02:00
Jeremy Rubin
c5b36b1c1b Mempool Update Cut-Through Optimization
Often when we're updating mempool entries we update entries that we
ultimately end up removing the updated entries shortly thereafter. This
patch makes it so that we filter for such entries a bit earlier in
processing, which yields a mild improvement for these cases, and is
negligible overhead otherwise.
2021-12-08 23:07:56 -08:00
glozow
b6002b07a3 MOVEONLY: update_lock_points to txmempool.h 2021-12-06 15:29:59 +00:00
glozow
ddd74ff65c clean up txmempool includes 2021-12-02 14:49:19 +00:00
glozow
c4efc4db54 change TestLockPointValidity to take a const reference
The lockpoints are not changed in this function.
There is no reason to pass a pointer.
2021-12-02 14:45:18 +00:00
glozow
a64078e385 Break validation <-> txmempool circular dependency
No behavior change.

Parameterize removeForReorg using a CChain and callable that
encapsulates validation logic.  The mempool shouldn't need to know a
bunch of details about coinbase maturity and lock finality. Instead,
just pass in a callable function that says true/false. Breaks circular
dependency by removing txmempool's dependency on validation.
2021-11-30 12:21:56 +00:00
glozow
1b3a11e126 MOVEONLY: TestLockPointValidity to txmempool 2021-11-30 11:56:46 +00:00
MarcoFalke
e2b5192d1c
Merge bitcoin/bitcoin#23211: refactor: move update_* structs from txmempool.h to .cpp file
65aaf9495d refactor: move `update_*` structs from txmempool.h to .cpp file (Sebastian Falbesoner)
9947ce6262 refactor: use const reference for parents in `CTxMemPool::UpdateAncestorsOf` (Sebastian Falbesoner)

Pull request description:

  These helpers are exclusively used in txmempool.cpp, hence they should also be moved there. The PR also contains a commit which fixes const-correctness for parents in `CTxMemPool::UpdateAncestorsOf` and declares them as reference to avoid a copy.

ACKs for top commit:
  promag:
    Code review ACK 65aaf9495d. Verified move-only commit locally.

Tree-SHA512: 7ce29f3ba0e68b5355001f27725b00f6d54cc993015356eb40b61b8cdd17db49b980f4c3d798c8e0c940d245dc3a72c474bb9ff3c0ee971ead450786076812c2
2021-11-03 08:59:03 +01:00
MarcoFalke
1847ce2d49
Merge bitcoin/bitcoin#23157: txmempool -/-> validation 1/2: improve performance of check() and remove dependency on validation
082c5bf099 [refactor] pass coinsview and height to check() (glozow)
ed6115f1ea [mempool] simplify some check() logic (glozow)
9e8d7ad5d9 [validation/mempool] use Spend/AddCoin instead of UpdateCoins (glozow)
09d18916af MOVEONLY: remove single-use helper func CheckInputsAndUpdateCoins (glozow)
e8639ec26a [mempool] remove now-unnecessary code (glozow)
54c6f3c1da [mempool] speed up check() by using coins cache and iterating in topo order (glozow)
30e240f65e [bench] Benchmark CTxMemPool::check() (glozow)
cb1407196f [refactor/bench] make mempool_stress bench reusable and parameterizable (glozow)

Pull request description:

  Remove the txmempool <-> validation circular dependency by removing txmempool's dependency on validation. There are two functions in txmempool that need validation right now: `check()` and `removeForReorg()`. This PR removes the dependencies in `check()`.

  This PR also improves the performance of `CTxMemPool::check()` by walking through the entries exactly once, in ascending ancestorcount order, which guarantees that we see parents before children.

ACKs for top commit:
  jnewbery:
    reACK 082c5bf099
  GeneFerneau:
    tACK [082c5bf](082c5bf099)
  rajarshimaitra:
    tACK 082c5bf099
  theStack:
    Code-review ACK 082c5bf099

Tree-SHA512: 40ac622af1627b5c3e6abb4f0f035d833265a8c5e8dc88faf5354875dfb5137f137825e54bbd2a2668ed37b145c5d02285f776402629f58596e51853a9a79d29
2021-10-25 15:21:27 +02:00
MarcoFalke
faeb9a5753
remove unused CTxMemPool::info(const uint256& txid) 2021-10-22 12:32:08 +02:00
glozow
4307849256 [mempool] delete exists(uint256) function
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.
2021-10-21 16:26:59 +01:00
Samuel Dobson
a0efe529e4 Fix outdated comments referring to ::ChainActive() 2021-10-12 14:36:51 +13:00
Sebastian Falbesoner
65aaf9495d refactor: move update_* structs from txmempool.h to .cpp file
These helpers are exclusively used in txmempool.cpp, hence they
should also be moved there.

Can be reviewed with "--color-moved=dimmed-zebra".
2021-10-07 03:26:08 +02:00
glozow
082c5bf099 [refactor] pass coinsview and height to check()
Removes check's dependency on validation.h
2021-10-04 15:00:28 +01:00
fanquake
d09071da5b
[MOVEONLY] consensus: move amount.h into consensus
Move amount.h to consensus/amount.h.
Renames, adds missing and removes uneeded includes.
2021-09-30 07:41:57 +08:00
MarcoFalke
fa08d4cfb1
Use C++11 member initializer in CTxMemPoolEntry
This removes a bunch of boilerplate, makes the code easier to read.
Also, C++11 member initialization avoids accidental uninitialized
members.

Can be reviewed with the git option "--word-diff-regex=." or with "git
difftool --tool=meld".
2021-09-21 16:04:27 +02:00
W. J. van der Laan
488e745560
Merge bitcoin/bitcoin#12677: RPC: Add ancestor{count,size,fees} to listunspent output
6cb60f3e6d doc/release-notes: Add new listunspent fields (Luke Dashjr)
0be2f17ef5 QA: Add tests for listunspent ancestor{count,size,fees} to mempool_packages (Luke Dashjr)
6966e80f45 RPC: Add ancestor{count,size,fees} to listunspent output (Luke Dashjr)
3f77dfdaf0 Expose ancestorsize and ancestorfees via getTransactionAncestry (Luke Dashjr)

Pull request description:

  Requested by a user

ACKs for top commit:
  prayank23:
    reACK 6cb60f3e6d
  fjahr:
    Code review re-ACK 6cb60f3e6d
  kiminuo:
    ACK [6cb60f3](6cb60f3e6d)
  achow101:
    Code Review ACK 6cb60f3e6d
  naumenkogs:
    ACK 6cb60f3e6d
  darosior:
    utACK 6cb60f3e6d

Tree-SHA512: 5d16e5799558691e5853ab7ea2cc85514cb45da3ce69134d855c71845beef32ec6af5ab28d4462683e9800c8ea126f162773a9d3d5660edac08fd8edbfeda173
2021-09-20 19:25:43 +02:00
glozow
c6e016aa13 [mempool] check ancestor/descendant limits for packages
When calculating ancestor/descendant counts for transactions in the
package, as a heuristic, count every transaction in the package as an
ancestor and descendant of every other transaction in the package.

This may overestimate, but will not underestimate, the
ancestor/descendant counts. This shortcut still produces an accurate
count for packages of 1 parent + 1 child.
2021-08-05 12:37:28 +01:00
glozow
f551841d3e [refactor] pass size/count instead of entry to CalculateAncestorsAndCheckLimits
This does not change existing behavior.
The ancestor/descendant limits are inclusive of the entries themselves,
but CalculateAncestorsAndCheckLimits() does not need access to them.
2021-08-05 12:37:28 +01:00
glozow
97dd1c729d MOVEONLY: add helper function for calculating ancestors and checking limits 2021-08-05 12:37:28 +01:00
Luke Dashjr
3f77dfdaf0 Expose ancestorsize and ancestorfees via getTransactionAncestry 2021-08-01 23:38:47 +00:00
glozow
e8ecc621be [refactor] comment/naming improvements 2021-06-02 09:40:40 +01:00
W. J. van der Laan
7257e50dba
Merge bitcoin/bitcoin#20833: rpc/validation: enable packages through testmempoolaccept
13650fe2e5 [policy] detect unsorted packages (glozow)
9ef643e21b [doc] add release note for package testmempoolaccept (glozow)
c4259f4b7e [test] functional test for packages in RPCs (glozow)
9ede34a6f2 [rpc] allow multiple txns in testmempoolaccept (glozow)
ae8e6df709 [policy] limit package sizes (glozow)
c9e1a26d1f [fuzz] add ProcessNewPackage call in tx_pool fuzzer (glozow)
363e3d916c [test] unit tests for ProcessNewPackage (glozow)
cd9a11ac96 [test] make submit optional in CreateValidMempoolTransaction (glozow)
2ef187941d [validation] package validation for test accepts (glozow)
578148ded6 [validation] explicit Success/Failure ctors for MempoolAcceptResult (glozow)
b88d77aec5 [policy] Define packages (glozow)
249f43f3cc [refactor] add option to disable RBF (glozow)
897e348f59 [coins/mempool] extend CCoinsViewMemPool to track temporary coins (glozow)
42cf8b25df [validation] make CheckSequenceLocks context-free (glozow)

Pull request description:

  This PR enables validation dry-runs of packages through the `testmempoolaccept` RPC. The expectation is that the results returned from `testmempoolaccept` are what you'd get from test-then-submitting each transaction individually, in that order (this means the package is expected to be sorted in topological order, for now at least). The validation is also atomic: in the case of failure, it immediately halts and may return "unfinished" `MempoolAcceptResult`s for transactions that weren't fully validated. The API for 1 transaction stays the same.

  **Motivation:**
  - This allows you to test validity for transaction chains (e.g. with multiple spending paths and where you don't want to broadcast yet); closes #18480.
  - It's also a first step towards package validation in a minimally invasive way.
  - The RPC commit happens to close #21074 by clarifying the "allowed" key.

  There are a few added restrictions on the packages, mostly to simplify the logic for areas that aren't critical to main package use cases:
  - No package can have conflicts, i.e. none of them can spend the same inputs, even if it would be a valid BIP125 replacement.
  - The package cannot conflict with the mempool, i.e. RBF is disabled.
  - The total count of the package cannot exceed 25 (the default descendant count limit), and total size cannot exceed 101KvB (the default descendant size limit).

  If you're looking for review comments and github isn't loading them, I have a gist compiling some topics of discussion [here](https://gist.github.com/glozow/c3acaf161c95bba491fce31585b2aaf7)

ACKs for top commit:
  laanwj:
    Code review re-ACK 13650fe2e5
  jnewbery:
    Code review ACK 13650fe2e5
  ariard:
    ACK 13650fe

Tree-SHA512: 8c5cbfa91a6c714e1c8710bb281d5ff1c5af36741872a7c5df6b24874d6272b4a09f816cb8a4c7de33ef8e1c2a2c252c0df5105b7802f70bc6ff821ed7cc1a2f
2021-05-27 22:40:24 +02:00
Anthony Towns
793b268284 txmempool: add thread safety annotations 2021-05-21 12:14:01 +10:00
glozow
897e348f59 [coins/mempool] extend CCoinsViewMemPool to track temporary coins 2021-05-20 21:34:31 +01:00
MarcoFalke
faa9ef49d1
fuzz: Add tx_pool fuzz targets 2021-03-18 18:43:52 +01:00
fanquake
ebc4ab721b
refactor: post Optional<> removal cleanups 2021-03-17 14:56:20 +08:00
fanquake
57e980d13c
scripted-diff: remove Optional & nullopt
-BEGIN VERIFY SCRIPT-
git rm src/optional.h

sed -i -e 's/Optional</std::optional</g' $(git grep -l 'Optional<' src)

sed -i -e 's/{nullopt}/{std::nullopt}/g' $(git grep -l 'nullopt' src)
sed -i -e 's/ nullopt;/ std::nullopt;/g' $(git grep -l 'nullopt' src)
sed -i -e 's/ nullopt)/ std::nullopt)/g' $(git grep -l 'nullopt' src)
sed -i -e 's/(nullopt)/(std::nullopt)/g' $(git grep -l 'nullopt' src)
sed -i -e 's/ nullopt,/ std::nullopt,/g' $(git grep -l 'nullopt' src)
sed -i -e 's/? nullopt :/? std::nullopt :/g' $(git grep -l 'nullopt' src)
sed -i -e 's/: nullopt}/: std::nullopt}/g' $(git grep -l 'nullopt' src)

sed -i -e '/optional.h \\/d' src/Makefile.am

sed -i -e '/#include <optional.h>/d' src/test/fuzz/autofile.cpp src/test/fuzz/buffered_file.cpp src/test/fuzz/node_eviction.cpp

sed -i -e 's/#include <optional.h>/#include <optional>/g' $(git grep -l '#include <optional.h>' src)
-END VERIFY SCRIPT-
2021-03-15 10:41:30 +08:00
Wladimir J. van der Laan
702cfc8c53
Merge #21055: [Bundle 3/n] Prune remaining g_chainman usage in validation functions
e11b649650 validation: CVerifyDB::VerifyDB: Use locking annotation (Carl Dong)
03f75c42e1 validation: Use existing chain member in CChainState::LoadGenesisBlock (Carl Dong)
5e4af77380 validation: Use existing chain member in CChainState::AcceptBlock (Carl Dong)
fee73347c0 validation: Pass in chain to FindBlockPos+SaveBlockToDisk (Carl Dong)
a9d28bcd8d validation: Use *this in CChainState::ActivateBestChainStep (Carl Dong)
4744efc9ba validation: Pass in chainstate to CTxMemPool::check (Carl Dong)
1fb7b2c595 validation: Use *this in CChainState::InvalidateBlock (Carl Dong)
8cdb2f7e58 validation: Move LoadBlockIndexDB to CChainState (Carl Dong)
8b99efbcc0 validation: Move invalid block handling to CChainState (Carl Dong)
2bdf37fe18 validation: Pass in chainstate to CVerifyDB::VerifyDB (Carl Dong)
31eac50c72 validation: Remove global ::VersionBitsTip{State,SinceHeight,Statistics} (Carl Dong)
63e4c7316a validation: Pass in chainstate to ::PruneBlockFilesManual (Carl Dong)
4bada76237 validation: Pass in chainstate to UpdateTip (Carl Dong)
a3ba08ba7d validation: Remove global ::{{Precious,Invalidate}Block,ResetBlockFailureFlags} (Carl Dong)
4927c9e699 validation: Remove global ::LoadGenesisBlock (Carl Dong)
9da106be4d validation: Check chain tip is non-null in CheckFinalTx (Carl Dong)

Pull request description:

  Overall PR: #20158 (tree-wide: De-globalize ChainstateManager)

  Based on:
  - [x] #20750 | [Bundle 2/n] Prune g_chainman usage in mempool-related validation functions

  Note to reviewers:
  1. This bundle may _apparently_ introduce usage of `g_chainman` or `::Chain(state|)Active()` globals, but these are resolved later on in the overall PR. [Commits of overall PR](https://github.com/bitcoin/bitcoin/pull/20158/commits)
  2. There may be seemingly obvious local references to `ChainstateManager` or other validation objects which are not being used in callers of the current function in question, this is done intentionally to **_keep each commit centered around one function/method_** to ease review and to make the overall change systematic. We don't assume anything about our callers. Rest assured that once we are considering that particular caller in later commits, we will use the obvious local references. [Commits of overall PR](https://github.com/bitcoin/bitcoin/pull/20158/commits)
  3. When changing a function/method that has many callers (e.g. `LookupBlockIndex` with 55 callers), it is sometimes easier (and less error-prone) to use a scripted-diff. When doing so, there will be 3 commits in sequence so that every commit compiles like so:
  1. Add `new_function`, make `old_function` a wrapper of `new_function`, divert all calls to `old_function` to `new_function` **in the local module only**
  2. Scripted-diff to divert all calls to `old_function` to `new_function` **in the rest of the codebase**
  3. Remove `old_function`

  Note to self:
  - [x] Address: https://github.com/bitcoin/bitcoin/pull/20750#discussion_r579400663

ACKs for top commit:
  laanwj:
    Code review ACK e11b649650

Tree-SHA512: 205a451a741e32f17d5966de289f2f5a3f0817738c0087b70ff4755ddd217b53d01050ed396669bda2b1d216a88d927b9778777f9ff95ab1fe20e59c5f341776
2021-03-04 14:55:47 +01:00
Carl Dong
4744efc9ba validation: Pass in chainstate to CTxMemPool::check
This is the only instance where validation reaches for something outside
of it.
2021-03-03 14:49:29 -05:00
Wladimir J. van der Laan
b59f2787e5
Merge #18017: txmempool: split epoch logic into class
fd6580e405 [refactor] txmempool: split epoch logic into class (Anthony Towns)

Pull request description:

  Splits the epoch logic introduced in #17925 into a separate class.

  Uses clang's thread safety annotations and encapsulates the data more strongly to reduce chances of bugs from API misuse.

ACKs for top commit:
  jonatack:
    ACK fd6580e405 using clang thread safety annotations looks like a very good idea, and the encapsulation this change adds should improve robustness (and possible unit test-ability) of the code. Verified that changing some of the locking duly provoked build-time warnings with Clang 9 on Debian and that small changes in the new `Epoch` class were covered by failing functional test assertions in `mempool_updatefromblock.py`, `mempool_resurrect.py`, and `mempool_reorg.py`
  hebasto:
    re-ACK fd6580e405, since my [previous](https://github.com/bitcoin/bitcoin/pull/18017#pullrequestreview-569619362) review:

Tree-SHA512: 7004623faa02b56639aa05ab7a078320a6d8d54ec62d8022876221e33f350f47df51ddff056c0de5be798f8eb39b5c03c2d3f035698555d70abc218e950f2f8c
2021-02-24 09:57:21 +01:00
Carl Dong
7142018812 validation: Pass in chainstate to CTxMemPool::removeForReorg
Several other parameters are now redundant since they can be safely
obtained from the chainstate given that ::cs_main is locked. These are
now removed.
2021-02-18 14:49:10 -05:00
Anthony Towns
fd6580e405 [refactor] txmempool: split epoch logic into class 2021-02-09 15:10:46 +10:00
MarcoFalke
fa362064e3
rpc: Return total fee in mempool
Also, add missing lock annotations
2021-01-28 10:43:22 +01:00
Carl Dong
b396467053 locks: Annotate CTxMemPool::check to require cs_main
Currently, CTxMemPool::check locks CTxMemPool's own cs member, then
calls GetSpendHeight which locks cs_main. This can potentially cause an
undesirable lock invesion since CTxMemPool's cs is supposed to be locked
after cs_main.

This does not cause us any problems right now because all callers of
CTxMemPool already lock cs_main before calling CTxMemPool::check, which
means that the LOCK(cs_main) in GetSpendHeight becomes benign.

However, it is currently possible for new code to be added which calls
CTxMemPool::check without locking cs_main (which would be dangerous).
Therefore we should make it explicit that cs_main needs to be held
before calling CTxMemPool::check.

NOTE: After all review-only assertions are removed in "#20158 |
      tree-wide: De-globalize ChainstateManager", and assuming that we
      keep the changes in "validation: Pass in spendheight to
      CTxMemPool::check", we can re-evaluate to see if this annotation
      is still necessary.
2021-01-20 16:15:03 -05:00
Wladimir J. van der Laan
8ffaf5c2f5
Merge #19935: Move SaltedHashers to separate file and add some new ones
281fd1a4a0 Replace KeyIDHasher with SaltedSipHasher (Andrew Chow)
210b693db6 Add generic SaltedSipHasher (Andrew Chow)
95e61c1cf2 Move Hashers to util/hasher.{cpp/h} (Andrew Chow)

Pull request description:

  There are existing `SaltedOutPointHasher` and `SaltedTxidHasher` classes used for `std::unordered_map` and `std::unordered_set` that could be useful in other places in the codebase. So we these to their own `saltedhash.{cpp/h}` file. An existing `KeyIDHasher` is moved there too. Additionally, `ScriptIDHasher`, `SaltedPubkeyHasher`, and `SaltedScriptHasher` are added so that they can be used in future work.

  `KeyIDHasher` and `ScriptIDHasher` are not salted so that equality comparisons of maps and sets keyed by `CKeyID` and `CScriptID` will actually work.

  Split from #19602 (and a few other PRs/branches I have).

ACKs for top commit:
  laanwj:
    Code review ACK 281fd1a4a0
  jonatack:
    ACK 281fd1a4a0, code review, debug build and ran bitcoind after rebasing to master @ dff0f6f753
  fjahr:
    utACK 281fd1a4a0

Tree-SHA512: bb03b231ccf3c9ecefc997b8da9c3770af4819f9be5b0a72997a103864e84046a2ac39b8eadf0dc9247bdccd53f86f433642e3a098882e6748341a9e7736271b
2021-01-13 08:49:17 +01:00
Fabian Jahr
1e62350ca2
refactor: Improve use of explicit keyword 2020-12-01 18:36:39 +01:00