Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

UTXO snapshot creation (dumptxoutset) #16899

Merged
merged 5 commits into from Nov 5, 2019

Conversation

@jamesob
Copy link
Member

jamesob commented Sep 17, 2019

This is part of the assumeutxo project:

Parent PR: #15606
Issue: #15605
Specification: https://github.com/jamesob/assumeutxo-docs/tree/master/proposal


This changeset defines the serialization format for UTXO snapshots and adds an RPC command for creating them, dumptxoutset. It also adds a convenience script for generating and verifying snapshots at a certain height, since that requires doing a hacky rewind of the chain via invalidateblock.

All of this is unused at the moment.

@jamesob jamesob force-pushed the jamesob:2019-09-au-dumptxoutset branch 2 times, most recently from fd56894 to e30e3a4 Sep 17, 2019
src/validation.h Outdated Show resolved Hide resolved
Copy link
Member

MarcoFalke left a comment

.

src/node/coinstats.h Outdated Show resolved Hide resolved
src/rpc/blockchain.cpp Outdated Show resolved Hide resolved
src/rpc/blockchain.cpp Show resolved Hide resolved
src/validation.h Outdated Show resolved Hide resolved
@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Sep 17, 2019

What was the conclusion on the question whether the dump should include all headers?

@jamesob jamesob force-pushed the jamesob:2019-09-au-dumptxoutset branch from e30e3a4 to 0705b18 Sep 17, 2019
@DrahtBot

This comment has been minimized.

Copy link
Contributor

DrahtBot commented Sep 17, 2019

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #16523 (Add removemempoolentry RPC to evict transactions from the mempool by metalicjames)
  • #16365 (Log RPC parameters (arguments) if -debug=rpcparams by LarryRuane)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@fanquake fanquake removed the Tests label Sep 17, 2019
@jamesob jamesob force-pushed the jamesob:2019-09-au-dumptxoutset branch 3 times, most recently from a71a387 to 6452a87 Sep 18, 2019
Copy link
Member Author

jamesob left a comment

What was the conclusion on the question whether the dump should include all headers?

We need to connect to peers to make the node usable anyway, and headers take very little time to sync, so I don't see much benefit to bundling the headers with the snapshot. Though if anyone thinks differently I'd be curious to hear why.

@jamesob jamesob force-pushed the jamesob:2019-09-au-dumptxoutset branch from 6452a87 to 52bba3c Sep 18, 2019
src/validation.h Outdated Show resolved Hide resolved
src/validation.h Outdated Show resolved Hide resolved
src/validation.h Outdated Show resolved Hide resolved
src/rpc/blockchain.cpp Outdated Show resolved Hide resolved

# TODO I'd like to make assertions about the contents of the file
# (ideally its hash), but because we can't get a deterministic block
# header (we'd need to mock time) and we have no way of deserializing

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Sep 24, 2019

Member

Why can't you use mock time here?

Also, the test isn't running (see travis)

This comment has been minimized.

Copy link
@jamesob

jamesob Sep 25, 2019

Author Member

To get a deterministic blockheader, we'd need to use GetTime<...>() here: https://github.com/bitcoin/bitcoin/blob/master/src/miner.cpp#L93

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Sep 25, 2019

Member

Huh, that nTimeStart is only used for logging benchmarks

This comment has been minimized.

Copy link
@jamesob

jamesob Sep 25, 2019

Author Member

Ah oops, you're totally right. Will try with mock.

Copy link
Contributor

ryanofsky left a comment

utACK 52bba3c

src/validation.h Outdated Show resolved Hide resolved
src/validation.h Outdated Show resolved Hide resolved
{
RPCHelpMan{
"dumptxoutset",
"\nWrite the serialized UTXO set to disk.\n"

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Sep 24, 2019

Contributor

In commit "rpc: add dumptxoutset" (b6af8f5)

Up to you, but I think documentation could say more about

  • Data format
  • What you'd use this function for, or it's mostly meant to be useful for developers
  • Status of the implementation. If this is experimental, whether the data format might change, if there's going to be a corresponding load rpc method
src/rpc/blockchain.cpp Outdated Show resolved Hide resolved
pcursor->Next();
}

afile.fclose();

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Sep 24, 2019

Contributor

In commit "rpc: add dumptxoutset" (b6af8f5)

Implementation seems fine for now. Unclear if in the future we might want to:

  • Put some magic bytes in snapshot header so to make it easier to recognize a snapshot file
  • Add version or length fields to make it possible to extend the format and include additional data
  • Write to temporary file and then rename in case theres a shutdown or crash in the middle of writing so you can distinguish an incomplete snapshot from a complete one
src/rpc/blockchain.cpp Outdated Show resolved Hide resolved
@jamesob jamesob force-pushed the jamesob:2019-09-au-dumptxoutset branch 3 times, most recently from 2894d3e to c2830a9 Sep 25, 2019
@Sjors

This comment has been minimized.

Copy link
Member

Sjors commented Sep 30, 2019

A progress indicator would be nice. If it wasn't for the system activity monitor showing CPU and disk write activity I would have interrupted the script somewhere during the ~ half an hour dumptxoutset takes.

Maybe the script should call setnetworkactive false. E.g. I'm seeing Warning: not punishing manually-connected peer, as well as a P2P timeout error.

Checksum on macOS 10.14.6 for block 597000: 3fd987a705a9b70f3197ebbe782ff457b1ea6ca373ac37b5c1a3764f7c414fe4 (produced twice, but on the same datadir)

{
  "coins_written": 62228899,
  "base_hash": "0000000000000000000eadb01d27f7b6ca1c17e71f0ff33853de9d3b25c57e55",
}
src/node/coinstats.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

ryanofsky left a comment

utACK c2830a9. Changes since last review: new utxo_snapshot.h file, removed hash and validation_complete snapshot metadata fields, fixed dumptxoutset base_height documentation, dumped to temporary file and renamed instead of writing directly, extended rpc_dumptxoutset test removing todo

laanwj added a commit that referenced this pull request Oct 2, 2019
2a4e60b Fix block index inconsistency in InvalidateBlock() (Suhas Daftuar)

Pull request description:

  Previously, we could release `cs_main` while leaving the block index in a state
  that would fail `CheckBlockIndex()`, because `setBlockIndexCandidates` was not being
  fully populated before releasing `cs_main`.

ACKs for top commit:
  TheBlueMatt:
    utACK 2a4e60b. I also discovered another issue in InvalidateBlock while reviewing, see #16856.
  Sjors:
    ACK 2a4e60b. Tested on top of #16899. Also tested `invalidateblock` with `-checkblockindex=1`.
  fjahr:
    ACK 2a4e60b. Ran tests, reviewed code, inspected behavior while manually testing `invalidateblock`.

Tree-SHA512: ced12f9dfff0d413258c709921543fb154789898165590b30d1ee0cdc72863382f189744f7669a7c924d3689a1cc623efdf4e5ae3efc60054572c1e6826de612
@jamesob jamesob force-pushed the jamesob:2019-09-au-dumptxoutset branch from 4e60702 to 95beaf6 Oct 24, 2019
@laanwj laanwj added this to the 0.20.0 milestone Oct 25, 2019
@jamesob jamesob force-pushed the jamesob:2019-09-au-dumptxoutset branch from 95beaf6 to 0efd43d Oct 25, 2019
@jamesob jamesob force-pushed the jamesob:2019-09-au-dumptxoutset branch from 0efd43d to 95c7f5e Oct 29, 2019
@DrahtBot DrahtBot removed the Needs rebase label Oct 29, 2019
Coin coin;

while (pcursor->Valid()) {
boost::this_thread::interruption_point();

This comment has been minimized.

Copy link
@laanwj

laanwj Oct 30, 2019

Member

There's no point in adding a boost interruption point here. RPC threads aren't managed by a boost thread group.
Check IsRPCRunning() instead.

This comment has been minimized.

Copy link
@jamesob

jamesob Nov 4, 2019

Author Member

Ah if that's the case then we should fix FindScriptPubKey at some point too. https://github.com/bitcoin/bitcoin/blob/master/src/rpc/blockchain.cpp#L1970-L1978

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Nov 5, 2019

Member

I wouldn't mind fixing it in this pull in a new commit. It seem like you copy-pasted the code from there, so it wouldn't hurt review if people saw where the code came from

laanwj added a commit to laanwj/bitcoin that referenced this pull request Oct 30, 2019
95c7f5e test: add dumptxoutset RPC test (James O'Beirne)
ddc90a8 devtools: add utxo_snapshot.sh (James O'Beirne)
49f281b rpc: add dumptxoutset (James O'Beirne)
644c7e7 coinstats: add coins_count (James O'Beirne)
707fde7 add unused SnapshotMetadata class (James O'Beirne)

Pull request description:

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

  Parent PR: bitcoin#15606
  Issue: bitcoin#15605
  Specification: https://github.com/jamesob/assumeutxo-docs/tree/master/proposal

  ---

  This changeset defines the serialization format for UTXO snapshots and adds an RPC command for creating them, `dumptxoutset`. It also adds a convenience script for generating and verifying snapshots at a certain height, since that requires doing a hacky rewind of the chain via `invalidateblock`.

  All of this is unused at the moment.
Copy link
Contributor

ryanofsky left a comment

Code review ACK 95c7f5e. Changes since last review: rebase, more stable setmocktime call

@jamesob jamesob force-pushed the jamesob:2019-09-au-dumptxoutset branch from 95c7f5e to 53bae65 Nov 4, 2019
@jamesob

This comment has been minimized.

Copy link
Member Author

jamesob commented Nov 4, 2019

au.dumptxoutset.13 -> au.dumptxoutset.14

Removed used of boost::interruption_point on @laanwj's advice.

Also changes existing CCoinsStats attributes to be C++11 initialized.
@jamesob jamesob force-pushed the jamesob:2019-09-au-dumptxoutset branch from 53bae65 to 4ebc0c1 Nov 5, 2019
jamesob added 3 commits Mar 28, 2019
Allows the creation of a UTXO snapshot to disk.
to allow easy (if not time-consuming) generation and verification of
snapshots.
@jamesob jamesob force-pushed the jamesob:2019-09-au-dumptxoutset branch from 4ebc0c1 to 92b2f53 Nov 5, 2019
@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Nov 5, 2019

We should remove all uses of boost::interruption_point in RPC code, but not in this PR.

ACK 92b2f53

laanwj added a commit that referenced this pull request Nov 5, 2019
92b2f53 test: add dumptxoutset RPC test (James O'Beirne)
c1ccbc3 devtools: add utxo_snapshot.sh (James O'Beirne)
57cf74c rpc: add dumptxoutset (James O'Beirne)
92fafb3 coinstats: add coins_count (James O'Beirne)
707fde7 add unused SnapshotMetadata class (James O'Beirne)

Pull request description:

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

  Parent PR: #15606
  Issue: #15605
  Specification: https://github.com/jamesob/assumeutxo-docs/tree/master/proposal

  ---

  This changeset defines the serialization format for UTXO snapshots and adds an RPC command for creating them, `dumptxoutset`. It also adds a convenience script for generating and verifying snapshots at a certain height, since that requires doing a hacky rewind of the chain via `invalidateblock`.

  All of this is unused at the moment.

ACKs for top commit:
  laanwj:
    ACK 92b2f53

Tree-SHA512: 200dff87767f157d627e99506ec543465d9329860a6cd49363081619c437163a640a46d008faa92b1f44fd403bfc7a7c9e851c658b5a4849efa9a34ca976bf31
@laanwj laanwj merged commit 92b2f53 into bitcoin:master Nov 5, 2019
0 of 2 checks passed
0 of 2 checks passed
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
echo
echo 'Examples:'
echo
echo " ./contrib/devtools/utxo_snapshot.sh 570000 utxo.dat ./src/bitcoin-cli -datadir=\$(pwd)/testdata"

This comment has been minimized.

Copy link
@Sjors

Sjors Nov 5, 2019

Member

I'm making a snapshot at height 600000, will share a torrent link later.

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Nov 5, 2019

Post merge ACK. Good stuff @jamesob

PIVOT_BLOCKHASH=$($BITCOIN_CLI_CALL getblockhash $(( GENERATE_AT_HEIGHT + 1 )) )

(>&2 echo "Rewinding chain back to height ${GENERATE_AT_HEIGHT} (by invalidating ${PIVOT_BLOCKHASH}); this may take a while")
${BITCOIN_CLI_CALL} invalidateblock "${PIVOT_BLOCKHASH}"

This comment has been minimized.

Copy link
@Sjors

Sjors Nov 5, 2019

Member

For others trying this: it's best to call setnetworkactive false first

sidhujag added a commit to syscoin/syscoin that referenced this pull request Nov 7, 2019
92b2f53 test: add dumptxoutset RPC test (James O'Beirne)
c1ccbc3 devtools: add utxo_snapshot.sh (James O'Beirne)
57cf74c rpc: add dumptxoutset (James O'Beirne)
92fafb3 coinstats: add coins_count (James O'Beirne)
707fde7 add unused SnapshotMetadata class (James O'Beirne)

Pull request description:

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

  Parent PR: bitcoin#15606
  Issue: bitcoin#15605
  Specification: https://github.com/jamesob/assumeutxo-docs/tree/master/proposal

  ---

  This changeset defines the serialization format for UTXO snapshots and adds an RPC command for creating them, `dumptxoutset`. It also adds a convenience script for generating and verifying snapshots at a certain height, since that requires doing a hacky rewind of the chain via `invalidateblock`.

  All of this is unused at the moment.

ACKs for top commit:
  laanwj:
    ACK 92b2f53

Tree-SHA512: 200dff87767f157d627e99506ec543465d9329860a6cd49363081619c437163a640a46d008faa92b1f44fd403bfc7a7c9e851c658b5a4849efa9a34ca976bf31
@fanquake fanquake added this to Done in Assume UTXO Nov 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.