Commit graph

908 commits

Author SHA1 Message Date
Anthony Towns
0cfd6c6a8f [refactor] versionbits: make VersionBitsCache a full class
Moves the VersionBits* functions to be methods of the cache class,
and makes the cache and its lock private to the class.
2021-06-30 08:19:12 +10:00
Anthony Towns
c64b2c6a0f scripted-diff: rename versionbitscache
-BEGIN VERIFY SCRIPT-
sed -i -e 's/versionbitscache/g_versionbitscache/g' $(git grep -l versionbitscache)
-END VERIFY SCRIPT-
2021-06-30 08:19:12 +10:00
Anthony Towns
de55304f6e [refactor] Add versionbits deployments to deploymentstatus.h
Adds support for versionbits deployments to DeploymentEnabled,
DeploymentActiveAfter and DeploymentActiveAt. Also moves versionbitscache
from validation to deploymentstatus.
2021-06-30 08:18:58 +10:00
Anthony Towns
2b0d291da8 [refactor] Add deploymentstatus.h
Provides DeploymentEnabled, DeploymentActiveAt, and DeploymentActiveAfter
helpers for checking the status of buried deployments. Can be overloaded
so the same syntax works for non-buried deployments, allowing future
soft forks to be changed from signalled to buried deployments without
having to touch the implementation code.

Replaces IsWitnessEnabled and IsScriptWitnessEnabled.
2021-06-29 17:11:12 +10:00
Anthony Towns
eccd736f3d versionbits: Use dedicated lock instead of cs_main 2021-06-29 17:11:12 +10:00
fanquake
8071ec179d
Merge bitcoin/bitcoin#21789: refactor: Remove ::Params() global from CChainState
fa0d9211ef refactor: Remove chainparams arg from CChainState member functions (MarcoFalke)
fa38947125 refactor: Remove ::Params() global from inside CChainState member functions (MarcoFalke)

Pull request description:

  The `::Params()` global is verbose and confusing. Also it makes tests a bit harder to write because they'd have to mock a global.

  Fix all issues by simply using a member variable that points to the right params.

  (Can be reviewed with `--word-diff-regex=.`)

ACKs for top commit:
  jnewbery:
    ACK fa0d9211ef
  kiminuo:
    utACK fa0d9211
  theStack:
    ACK fa0d9211ef 🍉

Tree-SHA512: 44676b19c9ed471ccb536331d3029bad192d7d50f394fd7b8527ec431452aeec8c4494164b9cf8e16e0123c4463b16be864366c6b599370032c17262625a0356
2021-06-29 11:22:57 +08:00
MarcoFalke
3f56ef7bef
Merge bitcoin/bitcoin#22146: Reject invalid coin height and output index when loading assumeutxo
fa9ebedec3 Reject invalid coin height and output index when loading assumeutxo (MarcoFalke)

Pull request description:

  It should be impossible to have a coin at a height higher than the height of the snapshot block, so reject those early to avoid integer wraparounds and hash collisions later on.

  Same for the outpoint index.

  Both issues were found by fuzzing:

  * The height issue by OSS-Fuzz: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=34793
  * The outpoint issue by my fuzz server: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=34793#c2

ACKs for top commit:
  practicalswift:
    cr ACK fa9ebedec3: patch looks correct
  jamesob:
    crACK fa9ebedec3
  theStack:
    Code review ACK fa9ebedec3
  benthecarman:
    crACK fa9ebedec3

Tree-SHA512: dae7caee4b3862b23ebdf2acb7edec4baf75b0dbf1409b370b1a73aa6b632b317ebfac596dcbaf4edfb1301b513f45465ea75328962460f35e2af0d7e547c9ac
2021-06-28 16:11:44 +02:00
MarcoFalke
fa0d9211ef
refactor: Remove chainparams arg from CChainState member functions
Passing this is confusing and redundant with the m_params member.
2021-06-13 09:43:54 +02:00
MarcoFalke
fa38947125
refactor: Remove ::Params() global from inside CChainState member functions
It is confusing and verbose to repeatedly access the global when a
member variable can simply refer to it.
2021-06-13 09:39:37 +02:00
MarcoFalke
9c1ec689f3
Merge bitcoin/bitcoin#22102: Remove Warning: from warning message printed for unknown new rules
6d7e46ce23 Remove `Warning:` (Prayank)

Pull request description:

  Reason: I noticed that `Warning` is printed 2 times in `-getinfo` while reviewing https://github.com/bitcoin/bitcoin/pull/21832#issuecomment-851004943

  Same string is used for GUI, log and stderr. If we need to add `Warning:` in GUI or other place we can always prepend to this string.

  CLI:

  ```
  Warnings: Unknown new rules activated (versionbit 28)

  ```

  GUI:

  ![image](https://user-images.githubusercontent.com/13405205/120110401-e36ab180-c18a-11eb-8031-4d52287dc263.png)

ACKs for top commit:
  MarcoFalke:
    review ACK 6d7e46ce23

Tree-SHA512: 25760cf6d850f6c2216d651fa46574d2d21a9d58f4577ecb58f9566ea8cd07bfcd6679bb8a1f53575e441b02d295306f492c0c99a48c909e60e5d977ec1861f6
2021-06-13 09:21:46 +02:00
Carl Dong
6f994882de validation: Farewell, global Chainstate! 2021-06-10 15:05:25 -04:00
Carl Dong
6c3b5dc0c1 scripted-diff: tree-wide: Remove all review-only assertions
-BEGIN VERIFY SCRIPT-
find_regex='((assert|CHECK_NONFATAL)\(std::addressof|TODO: REVIEW-ONLY)' \
    && git grep -l -E "$find_regex" -- . \
        | xargs sed -i -E "/${find_regex}/d"
-END VERIFY SCRIPT-
2021-06-10 15:05:24 -04:00
fanquake
ef8f2966ac
Merge bitcoin/bitcoin#22084: package testmempoolaccept followups
ee862d6efb MOVEONLY: context-free package policies (glozow)
5cac95cd15 disallow_mempool_conflicts -> allow_bip125_replacement and check earlier (glozow)
e8ecc621be [refactor] comment/naming improvements (glozow)
7d91442461 [rpc] reserve space in txns (glozow)
6c5f19d9c4 [package] static_assert max package size >= max tx size (glozow)

Pull request description:

  various followups from #20833

ACKs for top commit:
  jnewbery:
    utACK ee862d6efb
  ariard:
    Code Review ACK ee862d6

Tree-SHA512: 96ecb41f7bbced84d4253070f5274b7267607bfe4033e2bb0d2f55ec778cc41e811130b6321131e0418b5835894e510a4be8a0f822bc9d68d9224418359ac837
2021-06-10 19:09:54 +08:00
MarcoFalke
fa9ebedec3
Reject invalid coin height and output index when loading assumeutxo 2021-06-09 22:20:01 +02:00
MarcoFalke
82bc7faec8
Merge bitcoin/bitcoin#21946: Document and test lack of inherited signaling in RBF policy
2eb0eeda39 validation: document lack of inherited signaling in RBF policy (Antoine Riard)
906b6d9da6 test: Extend feature_rbf.py with no inherited signaling (Antoine Riard)

Pull request description:

  Contrary to BIP125 or other full-node implementation (e.g btcd), Bitcoin Core's mempool policy doesn't implement inherited signaling.

  This PR documents our mempool behavior on this and add a test demonstrating the case.

ACKs for top commit:
  jonatack:
    ACK 2eb0eeda39
  benthecarman:
    ACK 2eb0eeda39

Tree-SHA512: d41453d3b49bae3c1eb532a968f43bc047084913bd285929d4d9cba142777ff2be38163d912e28dfc635f4ecf446de68effad799c6e71be52f81e83410c712fb
2021-06-08 17:00:28 +02:00
Prayank
6d7e46ce23 Remove Warning:
+ Remove `Warning:` from warning message printed for unknown new rules
+ Change warning message in test

Author:    Prayank <prayank@tutanota.de>
2021-06-07 20:19:18 +05:30
fanquake
346e52afd6
Merge bitcoin/bitcoin#22121: doc: Various validation doc fixups
fa4245d884 doc: Various validation doc fixups (MarcoFalke)

Pull request description:

ACKs for top commit:
  michaelfolkson:
    Re-ACK fa4245d884
  jnewbery:
    ACK fa4245d884

Tree-SHA512: fa1086b09941247a4ffcbc1d7d27dc77a17a3ae093a5146dbb703db9ff4ba5d73ea77bd5b7747af79ea8a7dfe2c4c56a7e19ac5aac3417090e9ae127836022ae
2021-06-04 20:42:28 +08:00
Sjors Provoost
3d552b0d78
[doc] explain why CheckBlock() is called before AcceptBlock()
Co-authored-by: Suhas Daftuar <sdaftuar@gmail.com>
2021-06-03 19:09:28 +02:00
MarcoFalke
fa4245d884
doc: Various validation doc fixups
* Rename RewindBlockIndex -> NeedsRedownload (follow-up to commit
  d831e711ca)
* Fix typos
* Inline comments about faking chain data to avoid duplicating them
2021-06-03 13:53:31 +02:00
glozow
ee862d6efb MOVEONLY: context-free package policies
Co-authored-by: ariard <antoine.riard@gmail.com>
2021-06-02 17:26:44 +01:00
glozow
5cac95cd15 disallow_mempool_conflicts -> allow_bip125_replacement and check earlier 2021-06-02 09:52:50 +01:00
glozow
e8ecc621be [refactor] comment/naming improvements 2021-06-02 09:40:40 +01:00
fanquake
610151f5b0
validation: change ProcessNewBlock() to take a CBlock reference
Update ProcessNewBlock arguments to newer style.
2021-05-31 14:36:46 +08: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
glozow
13650fe2e5 [policy] detect unsorted packages 2021-05-24 15:48:32 +01:00
glozow
ae8e6df709 [policy] limit package sizes
Maximum number of transactions allowed in a package is 25, equal to the
default mempool descendant limit: if a package has more transactions
than this, either it would fail default mempool descendant limit or the
transactions don't all have a dependency relationship (but then they
shouldn't be in a package together). Same rationale for 101KvB virtual
size package limit.

Note that these policies are only used in test accepts so far.
2021-05-24 14:42:10 +01:00
glozow
2ef187941d [validation] package validation for test accepts
Only allow test accepts for now. Use the CoinsViewTemporary to keep
track of coins created by each transaction so that subsequent
transactions can spend them. Uncache all coins since we only
ever do test accepts (Note this is different from ATMP which doesn't
uncache for valid test_accepts) to minimize impact on the coins cache.

Require that the input txns have no conflicts and be ordered
topologically. This commit isn't able to detect unsorted packages.
2021-05-24 14:42:10 +01:00
Kiminuo
4c3a5dcbfc scripted-diff: Replace GetDataDir() calls with gArgs.GetDataDirNet() calls
-BEGIN VERIFY SCRIPT-
git ls-files -- 'src' ':(exclude)src/util/system.h' ':(exclude)src/util/system.cpp' | xargs sed -i 's/GetDataDir()/gArgs.GetDataDirNet()/g';
-END VERIFY SCRIPT-
2021-05-24 10:29:58 +02:00
glozow
578148ded6 [validation] explicit Success/Failure ctors for MempoolAcceptResult
Makes code more clear and prevents accidentally calling the wrong ctor.
2021-05-20 21:34:31 +01:00
glozow
249f43f3cc [refactor] add option to disable RBF
This is a mere refactor for now. We will use this to disable RBFing in
package validation.
2021-05-20 21:34:31 +01:00
glozow
42cf8b25df [validation] make CheckSequenceLocks context-free
Allow CheckSequenceLocks to use heights and coins from any CoinsView and
CBlockIndex provided. This means that CheckSequenceLocks() doesn't need
to hold the mempool lock or cs_main. The caller is responsible for
ensuring the CoinsView and CBlockIndex are consistent before passing
them in. The typical usage is still to create a CCoinsViewMemPool from
the mempool and grab the CBlockIndex from the chainstate tip.
2021-05-20 21:34:31 +01:00
Antoine Riard
2eb0eeda39 validation: document lack of inherited signaling in RBF policy 2021-05-14 14:27:30 -04:00
MarcoFalke
fa340b8794
refactor: Avoid magic value of all-zeros in assumeutxo base_blockhash
Just use std::optional
2021-05-11 11:21:05 +02:00
MarcoFalke
fae33f98e6
Fix assumeutxo crash due to invalid base_blockhash
Can be reviewed with --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space
2021-05-11 10:41:03 +02:00
MarcoFalke
fa5668bfb3
refactor: Use type-safe assumeutxo hash
This avoids accidentally mixing it up with other hashes (like block
hashes).
2021-05-11 10:40:40 +02:00
W. J. van der Laan
f8176b768a
Merge bitcoin/bitcoin#21836: scripted-diff: Replace three dots with ellipsis in the UI strings
d66f283ac0 scripted-diff: Replace three dots with ellipsis in the UI strings (Hennadii Stepanov)

Pull request description:

  This PR is split from #21463.
  The change was suggested on [Transifex.com](https://www.transifex.com/bitcoin/bitcoin/), and it does not touch `LogPrint` and `LogPrintf` calls.

  The only comment on #21463 [was](9030e4b5a6 (r597220100)):
  > Mind that these messages also end up in the log. In principle the log is already UTF-8 (as are all strings and text in bitcoind). But, just noting, that it might make browsing the log a less pleasant experience on systems with misconfigured locale like some BSDs by default.

ACKs for top commit:
  laanwj:
    ACK d66f283ac0

Tree-SHA512: 5ab1cb3160f3f996f1ad7d7486662da3eb7f06a857f4a1874963ce10caed5b86b0ad6151b1b9ebeb2b8aa5f0c85efad3b768ea9cafe5db86f78f88912b756d1e
2021-05-10 14:02:46 +02:00
MarcoFalke
128b98fce3
Merge bitcoin/bitcoin#21681: validation: fix ActivateSnapshot to use hardcoded nChainTx
91d93aac4e validation: remove nchaintx from assumeutxo metadata (James O'Beirne)
931684b24a validation: fix ActivateSnapshot to use hardcoded nChainTx (James O'Beirne)

Pull request description:

  This fixes an oversight from the move of nChainTx from the user-supplied
  snapshot metadata into the hardcoded assumeutxo chainparams.

  Since the nChainTx is now unused in the metadata, it should be removed
  in a future commit.

  See: https://github.com/bitcoin/bitcoin/pull/19806#discussion_r612165410

ACKs for top commit:
  Sjors:
    utACK 91d93aac4e
  ryanofsky:
    Code review ACK 91d93aac4e. No change to previous commit, just new commit removing now unused utxo snapshot field and updating tests.

Tree-SHA512: 445bdd738faf007451f40bbcf360dd1fb4675e17a4c96546e6818c12e33dd336dadd95cf8d4b5f8df1d6ccfbc4bf5496864bb5528e416cea894857b6b732140c
2021-05-05 18:34:16 +02:00
W. J. van der Laan
3275c6e578
Merge bitcoin/bitcoin#21727: refactor: Move more stuff to blockstorage
fa09a9eac8 style: Add { } to multi-line if (MarcoFalke)
fadafab833 move-only: Move functions to blockstorage (MarcoFalke)
fa7e64d586 move-only: Move constants to blockstorage (MarcoFalke)
fa247a327f refactor: Move block storage globals to blockstorage (MarcoFalke)
fa81c30c6f refactor: Move pruning/reindex/importing globals to blockstorage (MarcoFalke)

Pull request description:

  See #21575

ACKs for top commit:
  Sjors:
    ACK fa09a9eac8
  kiminuo:
    ACK fa09a9e
  laanwj:
    Code review ACK fa09a9eac8
  promag:
    Code review ACK fa09a9eac8. Since last review

Tree-SHA512: 2eb6962ff44da6b77f3058fc02ec66ab742e25ae8dcc8ec62b062896571910d43ca7c4bb16fb3ccb5e5245195b8dec6384b6c8d442fa97ca28d93bdff347d677
2021-05-05 16:03:03 +02:00
Hennadii Stepanov
d66f283ac0
scripted-diff: Replace three dots with ellipsis in the UI strings
-BEGIN VERIFY SCRIPT-
sed -i -E -e 's/\.\.\."\)(\.|,|\)| )/…"\)\1/' -- $(git ls-files -- 'src' ':(exclude)src/qt/bitcoinstrings.cpp')
sed -i -e 's/\.\.\.\\"/…\\"/' src/qt/sendcoinsdialog.cpp
sed -i -e 's|\.\.\.</string>|…</string>|' src/qt/forms/*.ui
sed -i -e 's|\.\.\.)</string>|…)</string>|' src/qt/forms/sendcoinsdialog.ui
-END VERIFY SCRIPT-
2021-05-02 22:17:16 +03:00
W. J. van der Laan
2b45cf0bcd
Merge bitcoin/bitcoin#19521: Coinstats Index
5f96d7d22d rpc: gettxoutsetinfo rejects hash_serialized_2 for specific height (Fabian Jahr)
23fe50436b test: Add test for coinstatsindex behavior in reorgs (Fabian Jahr)
90c966b0f3 rpc: Allow gettxoutsetinfo and getblockstats for stale blocks (Fabian Jahr)
b9362392ae index, rpc: Add use_index option for gettxoutsetinfo (Fabian Jahr)
bb7788b121 test: Test coinstatsindex robustness across restarts (Fabian Jahr)
e0938c2909 test: Add tests for block_info in gettxoutsetinfo (Fabian Jahr)
2501576ecc rpc, index: Add verbose amounts tracking to Coinstats index (Fabian Jahr)
655d929836 test: add coinstatsindex getindexinfo coverage, improve current tests (Jon Atack)
ca01bb8d68 rpc: Add Coinstats index to getindexinfo (Fabian Jahr)
57a026c30f test: Add unit test for Coinstats index (Fabian Jahr)
6a4c0c09ab test: Add functional test for Coinstats index (Fabian Jahr)
3f166ecc12 rpc: gettxoutsetinfo can be requested for specific blockheights (Fabian Jahr)
3c914d58ff index: Coinstats index can be activated with command line flag (Fabian Jahr)
dd58a4de21 index: Add Coinstats index (Fabian Jahr)
a8a46c4b3c refactor: Simplify ApplyStats and ApplyHash (Fabian Jahr)
9c8a265fd2 refactor: Pass hash_type to CoinsStats in stats object (Fabian Jahr)
2e2648a902 crypto: Make MuHash Remove method efficient (Fabian Jahr)

Pull request description:

  This is part of the coinstats index project tracked in #18000

  While the review of the new UTXO set hash algorithm (MuHash) takes longer recently #19328 was merged which added the possibility to run `gettxoutsetinfo` with a specific hash type. As the first type it added `hash_type=none` which skips the hashing of the UTXO set altogether. This alone did not make `gettxoutsetinfo` much faster but it allows the use of an index for the remaining coin statistics even before a new hashing algorithm has been added. Credit to Sjors for the idea to take this intermediate step.

  Features summary:
  - Users can start their node with the option `-coinstatsindex` which syncs the index in the background
  - After the index is synced the user can  use `gettxoutsetinfo` with `hash_type=none` or `hash_type=muhash` and will get the response instantly out of the index
  - The user can specify a height or block hash when calling `gettxoutsetinfo` to see coin statistics at a specific block height

ACKs for top commit:
  Sjors:
    re-tACK 5f96d7d22d
  jonatack:
    Code review re-ACK 5f96d7d22d per `git range-diff 13d27b4 07201d3 5f96d7d`
  promag:
    Tested ACK 5f96d7d22d. Light code review ACK 5f96d7d22d.

Tree-SHA512: cbca78bee8e9605c19da4fbcd184625fb280200718396c694a56c7daab6f44ad23ca9fb5456d09f245d8b8d9659fdc2b3f3ce5e953c1c6cf4003dbc74c0463c2
2021-04-30 17:27:19 +02:00
MarcoFalke
c6d6bc8abb
Merge bitcoin/bitcoin#21523: validation: run VerifyDB on all chainstates
844ad0ecca doc: IsSnapshotActive (James O'Beirne)
9b604c0207 validation: prepare VerifyDB for assumeutxo (James O'Beirne)
7901647d72 refactor: rename active_chainstate in VerifyDB (James O'Beirne)

Pull request description:

  This is part of the [assumeutxo project](https://github.com/bitcoin/bitcoin/projects/11) (parent PR: #15606)

  ---

  ~~Pretty cut and dry; parameterizes `CVerifyDB` methods so that we can run the verify procedure on multiple chainstates.~~

  Two minor tweaks to ensure that `VerifyDB` can be run on multiple chainstates and a corresponding rename.

ACKs for top commit:
  fjahr:
    Code review re-ACK 844ad0ecca
  MarcoFalke:
    review ACK 844ad0ecca 🐥

Tree-SHA512: 26a398cf4dabc1aa0850743921dba0452b4813848a3c777586dc981716737e98e17b8110254a5c41af95dd236e0c00dc8b4eee891d69bef825a5e1911fc499d0
2021-04-27 13:31:29 +02:00
MarcoFalke
fadafab833
move-only: Move functions to blockstorage 2021-04-27 10:36:23 +02:00
MarcoFalke
fa7e64d586
move-only: Move constants to blockstorage 2021-04-27 10:32:54 +02:00
MarcoFalke
fa247a327f
refactor: Move block storage globals to blockstorage
However, keep a declaration in validation to make it possible to move
smaller chunks to blockstorage without breaking compilation.

Also, expose AbortNode in the header.

Can be reviewed with --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space
2021-04-27 10:32:30 +02:00
MarcoFalke
fa81c30c6f
refactor: Move pruning/reindex/importing globals to blockstorage
Can be reviewed with --color-moved=dimmed-zebra
2021-04-27 10:32:24 +02:00
W. J. van der Laan
19a56d1519
Merge bitcoin/bitcoin#21009: Remove RewindBlockIndex logic
d831e711ca [validation] RewindBlockIndex no longer needed (Dhruv Mehta)

Pull request description:

  Closes #17862

  Context from [original comment](https://github.com/bitcoin/bitcoin/issues/17862#issuecomment-744285188) (minor edits):

  `RewindBlockIndex()` is a mechanism to allow nodes to be upgraded after segwit activation, while still keeping their chainstate/datadir in a consistent state. It works as follows:

  - A pre-segwit (i.e. v0.13.0 or older) node is running.
  -  Segwit activates. The pre-segwit node remains sync'ed to the tip, but is not enforcing the new segwit rules.
  - The user upgrades the node to a segwit-aware version (v0.13.1 or newer).
  - On startup, in `AppInitMain()`, `RewindBlockIndex()` is called. This walks the chain backwards from the tip, disconnecting and erasing blocks that from after segwit activation that weren't validated with segwit rules.
  - those blocks are then redownloaded (with witness data) and validated with segwit rules.

  This logic probably isn't required any more since:

  - Segwit activated at height 481824, when the block chain was 130GB and the total number of txs was 250 million. Today, we're at height 667704, the blockchain is over 315GB and the total number of txs is over 600 million. Even if 20% of that added data is witness data (a high estimate), then around 150GB of transactions would need to be rewound to get back to segwit activation height. It'd probably be faster to simply validate from genesis, especially since we won't be validating any scripts before the assumevalid block. It's also unclear whether rewinding 150GB of transactions would even work. It's certainly never been tested.
  - Bitcoin Core v0.13 is hardly used any more. https://luke.dashjr.org/programs/bitcoin/files/charts/software.html shows less than 50 nodes running it. The software was EOL on Aug 1st 2018. It's very unlikely that anyone is running 0.13 and will want to upgrade to 0.22.

  This PR introduces `NeedsRedownload()` which merely checks for insufficiently validated segwit blocks and requests that the user restarts the node with `-reindex`. Reindexing the block files upon restart will make the node rebuild chain state and block index from the `blk*.dat` files on disk. The node won't be able to index the blocks with `BLOCK_OPT_WITNESS`, so they will be missing from the chain and be re-downloaded, with witness data.

  Removing this code allows the following (done in follow-up #21090):

  - removal of tests using `segwitheight=-1` in `p2p_segwit.py`.
  - in turn, that allows us to drop support for `-segwitheight=-1`, which is only supported for that test.
  - that allows us to always set `NODE_WITNESS` in our local services. The only reason we don't do that is to support `-segwitheight=-1`.
  - that in turn allows us to drop all of the `GetLocalServices() & NODE_WITNESS` checks inside `net_processing.cpp`, since our local services would always include `NODE_WITNESS`

ACKs for top commit:
  jnewbery:
    utACK d831e711ca
  jamesob:
    ACK d831e711ca
  laanwj:
    Cursory code review ACK d831e711ca. Agree with the direction of the change, thanks for simplifying the logic here.
  glozow:
    utACK d831e711ca

Tree-SHA512: 3eddf5121ccd081ad7f15a5c6478ef867083edc8ba0bf1ee759e87bc070ee3d2f0698a3feba8db8dc087987c8452887b6f72cff05b3e178f41cb10a515fb8053
2021-04-27 10:14:52 +02:00
James O'Beirne
9b604c0207
validation: prepare VerifyDB for assumeutxo
Removes assumptions of use only on the active chainstate.
2021-04-23 15:06:48 -04:00
James O'Beirne
7901647d72
refactor: rename active_chainstate in VerifyDB
To prepare VerifyDB semantics for multiple
chainstate use.
2021-04-23 15:02:35 -04:00
Dhruv Mehta
d831e711ca [validation] RewindBlockIndex no longer needed
Instead of rewinding blocks, we request that the user restarts with
-reindex
2021-04-21 16:09:14 -07:00
Fabian Jahr
9c8a265fd2
refactor: Pass hash_type to CoinsStats in stats object 2021-04-19 20:28:48 +02:00
Kiminuo
b4190eff72 Change GetBlocksDir() to ArgsManager.GetBlocksDirPath(). 2021-04-18 11:59:28 +02:00
James O'Beirne
931684b24a
validation: fix ActivateSnapshot to use hardcoded nChainTx
This fixes an oversight from the move of nChainTx from the user-supplied
snapshot metadata into the hardcoded assumeutxo chainparams.

Since the nChainTx is now unused in the metadata, it should be removed
in a future commit.
2021-04-14 13:29:27 -04:00
MarcoFalke
a12962ca89
Merge #21585: Fix assumeutxo crash due to truncated file
fa73ce6e65 Fix assumeutxo crash due to truncated file (MarcoFalke)

Pull request description:

ACKs for top commit:
  jamesob:
    ACK fa73ce6e65
  ryanofsky:
    Code review ACK fa73ce6e65. Easy fix. It seems like this could have been caught in review, though.

Tree-SHA512: 3a98687c386e3995114ddf0ad7194fadd9520989290681ef703b578e3ca21aee51eadfb83aa38a489bac13d12709ea137b9b184b08e5bfa2919cca177aab90be
2021-04-14 15:12:14 +02:00
fanquake
1f14130cb0
Merge #21575: refactor: Create blockstorage module
fadcd3f78e doc: Remove irrelevant link to GitHub (MarcoFalke)
fa121b628d blockstorage: [refactor] Use chainman reference where possible (MarcoFalke)
fa0c7d9ad2 move-only: Move *Disk functions to blockstorage (MarcoFalke)
fa91b2b2b3 move-only: Move AbortNode to shutdown (MarcoFalke)
fa413f07a1 move-only: Move ThreadImport to blockstorage (MarcoFalke)
faf843c07f refactor: Move load block thread into ChainstateManager (MarcoFalke)

Pull request description:

  This picks up the closed pull request #21030 and is the first step toward fixing #21220.

  The basic idea is to move all disk access into a separate module with benefits:
  * Breaking down the massive files init.cpp and validation.cpp into logical units
  * Creating a standalone-module to reduce the mental complexity
  * Pave the way to fix validation related circular dependencies
  * Pave the way to mock disk access for testing, especially where it is performance critical (like fuzzing)

ACKs for top commit:
  promag:
    Code review ACK fadcd3f78e, checked (almost) moved only changes. This is a nice tidy up change and doesn't change behavior. Easily reviewed commit by commit.
  jamesob:
    ACK fadcd3f78e ([`jamesob/ackr/21575.1.MarcoFalke.refactor_create_blocksto`](https://github.com/jamesob/bitcoin/tree/ackr/21575.1.MarcoFalke.refactor_create_blocksto))
  ryanofsky:
    Code review ACK fadcd3f78e. New organization makes sense, moves extraneous things outside of validation.cpp. PR is also easy to review with helpfully split up moveonly commits.

Tree-SHA512: 917996592b6d8f9998289d8cb2b1b78b23d1fdb3b07216c9caec1380df33baa09dc2c1e706da669d440b497e79c9c62a01ca20dc202df5ad974a75f3ef7a143b
2021-04-13 22:00:28 +08:00
MarcoFalke
fa0c7d9ad2
move-only: Move *Disk functions to blockstorage
Can be reviewed with the git options
--color-moved=dimmed-zebra --color-moved-ws=ignore-all-space
2021-04-05 20:26:14 +02:00
MarcoFalke
fa91b2b2b3
move-only: Move AbortNode to shutdown
Can be reviewed with the git option
--color-moved=dimmed-zebra
2021-04-04 18:08:36 +02:00
MarcoFalke
fa9b74f5ea
Fix assumeutxo crash due to missing base_blockhash 2021-04-04 07:38:02 +02:00
MarcoFalke
fa73ce6e65
Fix assumeutxo crash due to truncated file 2021-04-03 17:52:58 +02:00
W. J. van der Laan
66daf4cb3b
Merge #21567: docs: fix various misleading comments
4eca20d6f7 [doc] correct comment about ATMPW (glozow)
8fa74aeb5b [doc] correct comment in chainparams (glozow)
2f8272c2a4 [doc] GetBestBlock() doesn't do nothing (gzhao408)

Pull request description:

  Came across a few misleading comments, wanted to fix them

ACKs for top commit:
  jnewbery:
    ACK 4eca20d6f7
  MarcoFalke:
    ACK 4eca20d6f7
  laanwj:
    Code review ACK 4eca20d6f7

Tree-SHA512: 5bef1f1e7703f304128cf0eb8945e139e031580c99062bbbe15bf4db8443c2ba5a8c65844833132e6646c8980c678fc1d2ab0c63e17105585d583570ee350fd0
2021-04-01 19:12:10 +02:00
glozow
4eca20d6f7 [doc] correct comment about ATMPW
ATMPW stands for AcceptToMemoryPoolWorker, which was removed in #16400.
2021-04-01 08:35:34 -07:00
gzhao408
2f8272c2a4 [doc] GetBestBlock() doesn't do nothing
This has tripped people up multiple times because it looks like
GetBestBlock is a const function returning the value of hashBlock.
2021-04-01 08:33:11 -07:00
Carl Dong
7e8b5ee814 validation: Make BlockManager::LookupBlockIndex const 2021-03-30 13:52:22 -04:00
fanquake
5294f0d5a9
refactor: return std::nullopt instead of {}
In #21415 we decided to return `std::optional` rather than `{}` for
uninitialized values. This PR repalces the two remaining usages of `{}`
with `std::nullopt`.

As a side-effect, this also quells the spurious GCC 10.2.x warning that
we've had reported quite a few times. i.e #21318, #21248, #20797.

```bash
txmempool.cpp: In member function ‘CTxMemPool::setEntries CTxMemPool::GetIterSet(const std::set<uint256>&) const’:
txmempool.cpp:898:13: warning: ‘<anonymous>’ may be used uninitialized in this function [-Wmaybe-uninitialized]
  898 |     return {};
      |             ^
```
2021-03-22 11:22:06 +08:00
MarcoFalke
63952f73b3
Merge #20921: validation: don't try to invalidate genesis block in CChainState::InvalidateBlock
787df19b09 validation: don't try to invalidate genesis block (Sebastian Falbesoner)

Pull request description:

  In the block invalidation method (`CChainState::InvalidateBlock`), the code for creating the candidate block map assumes that the passed block's previous block (`pindex->pprev`) is available and otherwise segfaults due to null-pointer deference in `CBlockIndexWorkComparator()` (see analysis by practicalswift in #20914), i.e. it doesn't work with the genesis block. Rather than analyzing all possible code paths and implications for this corner case, simply fail early if the genesis block is passed.

  Fixes #20914.

ACKs for top commit:
  sipa:
    ACK 787df19b09. Tested invalidation of generic on regtest.
  practicalswift:
    Tested ACK 787df19b09

Tree-SHA512: 978be7cf2bd1c1faebfe945d191ac77dea72791bea826459abd308f77c74c5991efee495a38817c306e488ecd5208b5c888df7d9d044132dd9a06bbbdb256b6c
2021-03-20 12:46:11 +01:00
Wladimir J. van der Laan
a9d1b40d53
Merge #21415: refactor: remove Optional & nullopt
ebc4ab721b refactor: post Optional<> removal cleanups (fanquake)
57e980d13c scripted-diff: remove Optional & nullopt (fanquake)

Pull request description:

  Same rationale & motivation as #21404, which turned out to be quite low in the number of potential conflicts. Lets see what the bot has to say here.

ACKs for top commit:
  practicalswift:
    cr ACK ebc4ab721b: patch looks correct
  jnewbery:
    utACK ebc4ab721b
  laanwj:
    Code review ACK ebc4ab721b

Tree-SHA512: 550fbeef09b9d35ddefaa805d1755c18c8fd499c4b0f77ebfece8c20296a7abd1cf6c699e2261f92fe3552deeb7555ec2a2287ffe3ab9e98bb9f8612a4d43be3
2021-03-17 12:17:33 +01:00
fanquake
ebc4ab721b
refactor: post Optional<> removal cleanups 2021-03-17 14:56:20 +08:00
MarcoFalke
67ec26cacf
Merge #19259: fuzz: Add fuzzing harness for LoadMempool(...) and DumpMempool(...)
68afd3eeec tests: Add fuzzing harness for LoadMempool(...) and DumpMempool(...) (practicalswift)
91af6b97c9 validation: Make DumpMempool(...) and LoadMempool(...) easier to test/fuzz/mock (practicalswift)
af322c7494 tests: Set errno in FuzzedFileProvider. Implement seek(..., ..., SEEK_END). (practicalswift)

Pull request description:

  Add fuzzing harness for `LoadMempool(...)` and `DumpMempool(...)`.

  See [`doc/fuzzing.md`](https://github.com/bitcoin/bitcoin/blob/master/doc/fuzzing.md) for information on how to fuzz Bitcoin Core. Don't forget to contribute any coverage increasing inputs you find to the [Bitcoin Core fuzzing corpus repo](https://github.com/bitcoin-core/qa-assets).

  Happy fuzzing :)

ACKs for top commit:
  jonatack:
    Tested re-ACK 68afd3eeec

Tree-SHA512: 4b5fcaa87e6eb478611d3b68eb6859645a5e121e7e3b056ad2815699dace0a6123706ff542def371b47f4ab3ce2b8a29782026d84fb505827121e9b4cc7dac31
2021-03-15 18:56:06 +01: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
MarcoFalke
e0bc27a14c
Merge #21404: refactor: Remove MakeUnique<T>()
1a6323bdbe doc: update developer notes for removal of MakeUnique (fanquake)
3ba2840e7e scripted-diff: remove MakeUnique<T>() (fanquake)

Pull request description:

  Since requiring C++17, this is just pointless abstraction. I think we should just "tear the band-aid off" and remove it. Similar to the changes happening in #21366.

  Also, having a comment saying this is deprecated doesn't prevent it's usage in new code. i.e : https://github.com/bitcoin/bitcoin/pull/20946#discussion_r561949731.

  The repository is fairly quiet at the moment, so any potential complaints about having to rebase should be minimal. Might as well get this over and done with.

ACKs for top commit:
  jnewbery:
    utACK 1a6323bdbe
  practicalswift:
    cr ACK 1a6323bdbe: patch looks correct
  ajtowns:
    ACK 1a6323bdbe -- code review only
  glozow:
    ACK 1a6323bdbe looks correct

Tree-SHA512: 4a14b9611b60b9b3026b54d6f5a2dce4c5d9b63a7b93d7de1307512df736503ed84bac66e7b93372c76e3117f49bf9f29cd473d3a47cb41fb2775bc10234736f
2021-03-12 08:34:15 +01:00
practicalswift
91af6b97c9 validation: Make DumpMempool(...) and LoadMempool(...) easier to test/fuzz/mock 2021-03-11 22:34:39 +00:00
fanquake
3ba2840e7e
scripted-diff: remove MakeUnique<T>()
-BEGIN VERIFY SCRIPT-
git rm src/util/memory.h
sed -i -e 's/MakeUnique/std::make_unique/g' $(git grep -l MakeUnique src)
sed -i -e '/#include <util\/memory.h>/d' $(git grep -l '#include <util/memory.h>' src)
sed -i -e '/util\/memory.h \\/d' src/Makefile.am
-END VERIFY SCRIPT-
2021-03-11 13:45:14 +08:00
Carl Dong
106bcd4f39 node/coinstats: Pass in BlockManager to GetUTXOStats 2021-03-08 15:54:31 -05:00
Carl Dong
e11b649650 validation: CVerifyDB::VerifyDB: Use locking annotation
...instead of recursively locking unconditionally
2021-03-03 14:56:26 -05:00
Carl Dong
03f75c42e1 validation: Use existing chain member in CChainState::LoadGenesisBlock 2021-03-03 14:49:30 -05:00
Carl Dong
5e4af77380 validation: Use existing chain member in CChainState::AcceptBlock 2021-03-03 14:49:30 -05:00
Carl Dong
fee73347c0 validation: Pass in chain to FindBlockPos+SaveBlockToDisk 2021-03-03 14:49:30 -05:00
Carl Dong
a9d28bcd8d validation: Use *this in CChainState::ActivateBestChainStep 2021-03-03 14:49:30 -05: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
Carl Dong
1fb7b2c595 validation: Use *this in CChainState::InvalidateBlock 2021-03-01 17:56:23 -05:00
Carl Dong
8cdb2f7e58 validation: Move LoadBlockIndexDB to CChainState
CChainState needed cuz setBlockIndexCandidates
2021-03-01 17:56:22 -05:00
Carl Dong
8b99efbcc0 validation: Move invalid block handling to CChainState
- InvalidChainFound
- CheckForkWarningConditions
2021-03-01 17:56:07 -05:00
Carl Dong
2bdf37fe18 validation: Pass in chainstate to CVerifyDB::VerifyDB 2021-03-01 17:56:07 -05:00
Carl Dong
31eac50c72 validation: Remove global ::VersionBitsTip{State,SinceHeight,Statistics}
Tip: versionbitscache is currently a global so we didn't need to pass it
     in to any of ::VersionBitsTip*'s callers
2021-03-01 17:56:07 -05:00
Carl Dong
63e4c7316a validation: Pass in chainstate to ::PruneBlockFilesManual 2021-03-01 17:56:07 -05:00
Carl Dong
4bada76237 validation: Pass in chainstate to UpdateTip 2021-03-01 17:56:07 -05:00
Carl Dong
a3ba08ba7d validation: Remove global ::{{Precious,Invalidate}Block,ResetBlockFailureFlags} 2021-02-22 11:48:39 -05:00
Carl Dong
4927c9e699 validation: Remove global ::LoadGenesisBlock 2021-02-22 11:48:39 -05:00
Carl Dong
9da106be4d validation: Check chain tip is non-null in CheckFinalTx
...also update comments to remove mention of ::ChainActive()

From: https://github.com/bitcoin/bitcoin/pull/20750#discussion_r579400663

> Also, what about passing a const reference instead of a pointer? I
> know this is only theoretical, but previously if the tip was nullptr,
> then Height() evaluated to -1, now it evaluates to UB
2021-02-22 11:46:37 -05:00
MarcoFalke
34d7030063
Merge #21202: [validation] Two small clang lock annotation improvements
25c57d6409 [doc] Add a note about where lock annotations should go. (Amiti Uttarwar)
ad5f01b960 [validation] Move the lock annotation from function definition to declaration (Amiti Uttarwar)

Pull request description:

  Based on reviewing #21188

  the first commit switches the lock annotations on `CheckInputScripts` to be on the function declaration instead of on the function definition. this ensures that all call sites are checked, not just ones that come after the definition.

  the second commit adds a note to the developer-notes section to clarify where the annotations should be applied.

ACKs for top commit:
  MarcoFalke:
    ACK 25c57d6409 🥘
  promag:
    Code review ACK 25c57d6409.

Tree-SHA512: 61b6ef856bf6c6016d535fbdd19daf57b9e59fe54a1f30d47282a071b9b9d60b2466b044ee57929e0320cb1bdef52e7a1687cacaa27031bbc43d058ffffe22ba
2021-02-22 09:47:15 +01:00
Carl Dong
e8ae1db864 style-only: Make AcceptToMemoryPool signature readable 2021-02-18 14:49:10 -05:00
Carl Dong
8f5c100064 style-only: Make CheckSequenceLock signature readable 2021-02-18 14:49:10 -05:00
Carl Dong
8c824819c8 validation: Use *this in CChainState::LoadMempool 2021-02-18 14:49:10 -05:00
Carl Dong
0a9a24d8c7 validation: Pass in chainstate to UpdateMempoolForReorg 2021-02-18 14:49:10 -05: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
Carl Dong
71734c65dc validation: Pass in chain to ::TestLockPointValidity 2021-02-18 14:49:10 -05:00
Carl Dong
417dafc1ee validation: Remove old AcceptToMemoryPool w/o chainstate param 2021-02-18 14:49:10 -05:00
Carl Dong
229bc37b5f validation: Pass in chainstate to ::AcceptToMemoryPool 2021-02-18 14:43:28 -05:00
Carl Dong
d0da7ea57a validation: Pass in chainstate to ::LoadMempool 2021-02-18 14:43:28 -05:00
Carl Dong
3a205c43dc validation: Pass in chainstate to AcceptToMemoryPoolWithTime 2021-02-18 14:43:28 -05:00
Carl Dong
d8a816329c validation: Add chainstate member to MemPoolAccept 2021-02-18 14:43:28 -05:00