doc: add assumeutxo notes #23154
doc: add assumeutxo notes #23154
Conversation
|
|
||
| **Note:** at this point, ValidationInterface callbacks will be coming in from both | ||
| chainstates. Considerations here must be made for indexing, which may no longer be happening | ||
| sequentially. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Good point - I'll verify that I'm making a note somewhere in comments about this.
|
|
||
| When the bitcoind initializes again, what began as the snapshot chainstate is now | ||
| indistinguishable from a chainstate that has been built from the traditional IBD | ||
| process, and will be initialized as such. |
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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.
| chainstates. Considerations here must be made for indexing, which may no longer be happening | ||
| sequentially. | ||
|
|
||
| ### Background chainstate hits snapshot base block |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Yeah, I had been calling it IBD chainstate, but eventually standardized on background chainstate to cut down on jargon (see #15606 (review)).
jamesob
force-pushed
the
2021-10-assumeutxo-doc
branch
from
4c29459 to
9ab4401
Compare
|
ACK 9ab4401 |
1 similar comment
|
ACK 9ab4401 |
|
Anything left to do here? Seems like a pretty low-risk merge given it's just doc. |
|
ACK 9ab4401 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. |
|
|
||
| The RPC commands `dumptxoutset` and `loadtxoutset` are used to respectively generate | ||
| and load UTXO snapshots. The utility script `./contrib/devtools/utxo_snapshot.sh` may | ||
| be of use. |
There was a problem hiding this comment.
Would be good to tell the reader for what it is of use:
"[...]may be of use to create or validate historic snapshots."
| - A new block index `nStatus` flag is introduced, `BLOCK_ASSUMED_VALID`, to mark block | ||
| index entries that are required to be assumed-valid by a chainstate created | ||
| from a UTXO snapshot. This flag is mostly used as a way to modify certain | ||
| CheckBlockIndex() logic to account for index entries that are pending validation by a |
There was a problem hiding this comment.
I would like some backticks at least for the functions: CheckBlockIndex() and LoadBlockIndex() below.
There was a problem hiding this comment.
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.
|
|
||
| ## Design notes | ||
|
|
||
| - A new block index `nStatus` flag is introduced, `BLOCK_ASSUMED_VALID`, to mark block |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
re: #23154 (comment)
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.
|
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). |
This is part of the assumeutxo project (parent PR: #15606)
Adds some notes on assumeutxo design.
Related: #21526 (comment)