doc: add assumeutxo notes #23154

jamesob commented at 2:25 PM on October 1, 2021: member

This is part of the assumeutxo project (parent PR: #15606)

Adds some notes on assumeutxo design.

DrahtBot added the label Docs on Oct 1, 2021

in doc/assumeutxo.md:67 in 4c29459b44 outdated

  62 | +| number of chainstates | 1 |
  63 | +| active chainstate | ibd |
  64 | +
  65 | +### User loads a UTXO snapshot via `loadtxoutset` RPC
  66 | +
  67 | +`ChainstateManager` initializes a new chainstate to load the snapshot contents into.

ariard commented at 6:32 PM on October 3, 2021:

"see ActivateSnapshot()" ?

in doc/assumeutxo.md:89 in 4c29459b44 outdated

  84 | +| number of chainstates | 2 |
  85 | +| active chainstate | snapshot |
  86 | +
  87 | +The snapshot begins to sync to tip from its base block, technically in parallel with
  88 | +the original chainstate, but it is given priority during block download and is
  89 | +allocated most of the cache as our chief consideration is getting to network tip.

ariard commented at 6:32 PM on October 3, 2021:

"see MaybeRebalanceCaches" ? ?

in doc/assumeutxo.md:104 in 4c29459b44 outdated

  98 | +to the background chainstate, which is responsible for doing full validation of the
  99 | +assumed-valid parts of the chain.
 100 | +
 101 | +**Note:** at this point, ValidationInterface callbacks will be coming in from both
 102 | +chainstates. Considerations here must be made for indexing, which may no longer be happening
 103 | +sequentially.

ariard commented at 6:38 PM on October 3, 2021:

Do you have a Big Comment(tm) warning users of ValidationInterface callbacks of this subtlety ? Grepping quickly src/validationinterface.h, I don't see a mention of background/snapshot chainstate. Could be nice to be added at somepoint.

jamesob commented at 8:54 PM on October 4, 2021:

Good point - I'll verify that I'm making a note somewhere in comments about this.

in doc/assumeutxo.md:108 in 4c29459b44 outdated

 103 | +sequentially.
 104 | +
 105 | +### Background chainstate hits snapshot base block
 106 | +
 107 | +Once the tip of the background chainstate hits the base block of the snapshot
 108 | +chainstate, we stop use of the background chainstate (by setting `m_stop_use` in

ariard commented at 6:41 PM on October 3, 2021:

I think m_stop_use isn't yet landed. Maybe add a "//XXX:" pointing to the downstream branch where it's present ? Like docs isn't matching yet the code.

in doc/assumeutxo.md:133 in 4c29459b44 outdated

 127 | +
 128 | +### Bitcoind restarts sometime after snapshot validation has completed
 129 | +
 130 | +When the bitcoind initializes again, what began as the snapshot chainstate is now
 131 | +indistinguishable from a chainstate that has been built from the traditional IBD
 132 | +process, and will be initialized as such.

ariard commented at 6:44 PM on October 3, 2021:

From a user, observing the chainstate from RPC calls, do you have any difference with the above "Normal" operation" at that stage or they're fully similar ?

jamesob commented at 8:39 PM on October 4, 2021:

At the stage they're fully similar, but we could probably leave behind some indication that a snapshot was used if that's preferable for some reason. But at the moment I don't think there are any artifacts in the block index that could be used to indicate this.

ariard commented at 6:45 PM on October 3, 2021: member

SGTM overall, when the assumeutxo code is already present, I checked that the description is matching the doc.

doc: add assumeutxo notes 9ab440199d

in doc/assumeutxo.md:106 in 4c29459b44 outdated

 100 | +
 101 | +**Note:** at this point, ValidationInterface callbacks will be coming in from both
 102 | +chainstates. Considerations here must be made for indexing, which may no longer be happening
 103 | +sequentially.
 104 | +
 105 | +### Background chainstate hits snapshot base block

naumenkogs commented at 10:54 AM on October 4, 2021:

Perhaps you could call this ibd chainstate, and not background? Or anything not background, because at some point background also refers to the snapshot as well

jamesob commented at 2:40 PM on October 4, 2021:

Yeah, I had been calling it IBD chainstate, but eventually standardized on background chainstate to cut down on jargon (see #15606#pullrequestreview-692965905).

jamesob force-pushed on Oct 4, 2021

fanquake added this to the "In progress" column in a project

ariard commented at 10:22 PM on October 25, 2021: member

ACK 9ab4401

naumenkogs commented at 8:03 AM on October 26, 2021: member

ACK 9ab4401

jamesob commented at 1:55 PM on October 29, 2021: member

Anything left to do here? Seems like a pretty low-risk merge given it's just doc.

michaelfolkson commented at 2:31 PM on October 29, 2021: contributor

ACK 9ab440199d5c888363a42c957433d0e46cd0d2ff

Agree this is a low risk merge. To be really finicky maybe a new standalone doc for any project should only be merged once the project is completed (fully merged) and functional. But it can obviously be easily be reverted if for some reason the project wasn't completed/fully merged.

edit: I do think docs and future GUI changes will be important for AssumeUTXO on completion (assuming it is fully merged). How it should be used, what is encouraged, what warnings are displayed to the user etc. We do (imo) want to do everything that we can to ensure the user does full IBD from genesis in the background rather than skipping it and is aware of the risks of sending or receiving funds before full IBD has completed. But that discussion is for later.

in doc/assumeutxo.md:8 in 9ab440199d

   0 | @@ -0,0 +1,138 @@
   1 | +# assumeutxo
   2 | +
   3 | +Assumeutxo is a feature that allows fast bootstrapping of a validating bitcoind
   4 | +instance with a very similar security model to assumevalid.
   5 | +
   6 | +The RPC commands `dumptxoutset` and `loadtxoutset` are used to respectively generate
   7 | +and load UTXO snapshots. The utility script `./contrib/devtools/utxo_snapshot.sh` may
   8 | +be of use.

fjahr commented at 8:28 PM on November 2, 2021:

Would be good to tell the reader for what it is of use: "[...]may be of use to create or validate historic snapshots."

in doc/assumeutxo.md:21 in 9ab440199d

  16 | +## Design notes
  17 | +
  18 | +- A new block index `nStatus` flag is introduced, `BLOCK_ASSUMED_VALID`, to mark block
  19 | +  index entries that are required to be assumed-valid by a chainstate created
  20 | +  from a UTXO snapshot. This flag is mostly used as a way to modify certain
  21 | +  CheckBlockIndex() logic to account for index entries that are pending validation by a

fjahr commented at 8:29 PM on November 2, 2021:

I would like some backticks at least for the functions: CheckBlockIndex() and LoadBlockIndex() below.

fjahr approved

fjahr commented at 9:49 PM on November 2, 2021: member

ACK 9ab440199d5c888363a42c957433d0e46cd0d2ff

I think this will be very helpful for reviewers new to the project.

Suggested improvements can be left for future changes as this doc will need to be kept up to date as the project progresses anyway.

in doc/assumeutxo.md:18 in 9ab440199d

  13 | +- [Github issue](https://github.com/bitcoin/bitcoin/issues/15605)
  14 | +- [draft PR](https://github.com/bitcoin/bitcoin/pull/15606)
  15 | +
  16 | +## Design notes
  17 | +
  18 | +- A new block index `nStatus` flag is introduced, `BLOCK_ASSUMED_VALID`, to mark block

MarcoFalke commented at 7:41 AM on November 3, 2021:

I am wondering who is the audience of this document?

If it is a developer, it might be better to put the documentation in the source code. Otherwise it will be hard for developers to find it and it will more easily get out of date.

If the target audience is a user, I am wondering why internal implementation details are mentioned and why they are relevant to the user.

If the target audience is a reviewer of your pull request, I am wondering why this needs a doc at all in the source tree. Wouldn't a pull request description or so be a better place?

ryanofsky commented at 11:42 AM on November 3, 2021:

re: #23154 (review)

I am wondering who is the audience of this document? [...]

I think the concerns here go a little beyond scope of this PR. Some of the files in the doc/ folder are clearly targeted at developers and not helpful to users like https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md. Some files like https://github.com/bitcoin/bitcoin/blob/master/doc/JSON-RPC-interface.md and the release notes are mostly useful for end users, and definitely intended to be read by them. I think ideally someone would go through the doc folder, keeping the files that are intended to be read by users but moving the other ones that are intended for developers/testers/contributors somewhere else like doc/devel,doc/internal, doc/contrib, doc/design.

This document is a basically a design document (other examples: golang, chromium) intended to by read by developers. It should be useful not just to people reviewing the PR, but also anybody going forward who want to understand how this feature which touches different parts of the source code is implemented and what some of design considerations are.

At the top of this file, there is a small amount of information that could be useful to users, but it is brief and basically just linking to RPCs and a script. If bitcoin had a user manual like many other projects do, this section could be copied there. But bitcoin doesn't have a user manual so, 🤷

If it is a developer, it might be better to put the documentation in the source code. Otherwise it will be hard for developers to find it and it will more easily get out of date.

I think this could be addressed by organizing the doc directory a little better and linking to the document from the source code so it is easier to find.

If the target audience is a user, I am wondering why internal implementation details are mentioned and why they are relevant to the user.

Users are not target audience here, AFAICT.

If the target audience is a reviewer of your pull request, I am wondering why this needs a doc at all in the source tree. Wouldn't a pull request description or so be a better place?

I think this is a false dichotomy. If I am reviewing a pull request that includes end-user release notes, those release notes are usually more helpful to me for understanding behavior of the PR than any of the weedsy implementation details in the PR description or other parts of the diff.

PR descriptions do tend to be better places than design documents for certain information, like describing details or drawbacks of previous code, but design documents are useful both for current developers reviewing changes, and for future developers want to understand how a feature works. It is not an either/or.

MarcoFalke commented at 7:46 AM on November 3, 2021: member

While this might be fine to merge, I disagree with the sentiment that this should be merged because it "is just documentation and nothing can go wrong". Anything can go wrong if the documentation is wrong. For example, loss of privacy with the wrong settings or loss of funds with the wrong wallet handling documentation.

Also, I am wondering about the audience of this doc.

Our current doc/ folder is a mess, mixing dev docs with user docs, without any structure. Surely having documentation is important, but I suspect that unstructured documentation is potentially worse than no documentation at all.

MarcoFalke merged this on Nov 3, 2021

MarcoFalke closed this on Nov 3, 2021

michaelfolkson commented at 12:15 PM on November 3, 2021: contributor

I think @MarcoFalke and @ryanofsky have some good points above on docs generally so added their comments to #20132. This doc and the docs generally are definitely a work in progress with lots of scope for improvement. I agree with this being merged though as James' first attempt at a doc for Assume UTXO as there are no inaccuracies or clear sources of confusion (as far as I can tell) and follow up PRs can improve it (e.g. target it directly at developers or users).

sidhujag referenced this in commit e1ca4d1318 on Nov 3, 2021

fanquake moved this from the "In progress" to the "Done" column in a project

DrahtBot locked this on Nov 3, 2022