doc: BaseIndex sync behavior with empty datadir #22485

jamesob · 2021-07-18T16:04:45Z

Make a note about a potentially confusing behavior with BaseIndex::m_synced;
if the user starts bitcoind with an empty datadir and an index enabled,
BaseIndex will consider itself synced (as a degenerate case). This affects
how indices are built during IBD (relying solely on BlockConnected signals vs.
using ThreadSync()).

Bocoi5011

.

DrahtBot · 2021-07-19T00:04:18Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#25193 (indexes: Read the locator's top block during init, allow interaction with reindex-chainstate by mzumsande)
#24230 (indexes: Stop using node internal types and locking cs_main, improve sync logic by ryanofsky)

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.

maflcko · 2021-07-19T08:52:23Z

src/index/base.cpp

+
+    // Note: this will latch to true immediately if the user starts up with an empty
+    // datadir and an index enabled. If this is the case, indexation will happen solely
+    // via `BlockConnected` signals until, possibly, the next restart.


What do you mean with "until the next restart"? Once the index is in sync with the active chain tip, it will stay that way, regardless of restarts. The only way it could get out of sync is when you disable the index on a restart, advance the tip, then enable it again. Am I missing something?

This is contingent on how fast the particular indexing process is; you can imagine that if a given index takes (for the sake of argument) a minute per block to build, if we get 1000 blocks connected and then the user shuts down immediately, the validationinterface queue may not clear* and we will start up out of sync after having been "synced" in the degenerate way. Does this make sense?

*I know we wait for this queue to clear during a graceful shutdown, but we can't assume this.

In commit "index: doc/log BaseIndex sync behavior with empty datadir" (2b48b03)

I also found this confusing and would suggest just saying "the index will get updated and stay in sync through BlockConnected notifications"

My issue with your suggested comment is that that isn't a complete picture of what's going on - as described above, indexation will only happen via BlockConnectected notifications until the next restart. If we have a hard shutoff that doesn't clear the validation queue, the index will sync sequentially in ThreadSync(), which makes that comment somewhat misleading.

This is fine. I think I'm just agreeing with Marco that the comment seemed a little strange because I didn't know what it might be implying. It seems obvious there will be no BlockConnected signals if the process is dead.

tryphe · 2021-07-30T10:14:50Z

Concept ACK

Any easy way to confirm this behavior, regarding BlockConnected signals vs.
using ThreadSync()? Would be willing to test it out.

jamesob · 2021-10-29T13:54:18Z

Worth merging? Anything left to do here? Should I close?

ryanofsky

Code review ACK 2b48b03. Concept ~0. I'd be curious to know more about what is confusing without this log message. It seems pretty straightforward that if there are no blocks, then there is nothing for the sync thread to do, and the index is in sync. Like if pieces_of_popcorn == 0 then popcorn_eaten == true. So I'm not sure why this is confusing or degenerate, etc.

I guess I like the new comments, but I'm just unsure what the log message adds.

ryanofsky · 2021-11-02T19:27:29Z

src/index/base.cpp

+
+    // Note: this will latch to true immediately if the user starts up with an empty
+    // datadir and an index enabled. If this is the case, indexation will happen solely
+    // via `BlockConnected` signals until, possibly, the next restart.


In commit "index: doc/log BaseIndex sync behavior with empty datadir" (2b48b03)

I also found this confusing and would suggest just saying "the index will get updated and stay in sync through BlockConnected notifications"

jamesob · 2022-01-10T20:58:00Z

Reopening as I think this is still a useful clarification when trying to remind myself of how indexation works.

maflcko · 2022-01-11T08:59:31Z

It is recommended to address the feedback comments in some way before merge

jamesob · 2022-01-11T14:16:37Z

I guess I like the new comments, but I'm just unsure what the log message adds.

It's obviously not mandatory, but I found the whole indexation process a little confusing and so I figured the more hints the better?

mzumsande

ACK to the comment, I think that the additional log message would be redundant (see below).

src/index/base.cpp

ryanofsky

Code review ACK 2b48b03. This seems fine and generally better, despite confusion and nitpicking

ryanofsky · 2022-03-21T19:49:07Z

src/index/base.cpp

+
+    // Note: this will latch to true immediately if the user starts up with an empty
+    // datadir and an index enabled. If this is the case, indexation will happen solely
+    // via `BlockConnected` signals until, possibly, the next restart.


This is fine. I think I'm just agreeing with Marco that the comment seemed a little strange because I didn't know what it might be implying. It seems obvious there will be no BlockConnected signals if the process is dead.

src/index/base.cpp

fjahr

utACK 2b48b03

I think this can be merged as is but I would prefer it without the log message (see my comment).

I think one more place where a note on this could be helpful for users is in the getindexinfo RPC, something like this: fjahr@255f094. Feel free to add this in here but I can also open a follow-up.

src/index/base.cpp

Make a note about a potentially confusing behavior with `BaseIndex::m_synced`; if the user starts bitcoind with an empty datadir and an index enabled, BaseIndex will consider itself synced (as a degenerate case). This affects how indices are built during IBD (relying solely on BlockConnected signals vs. using ThreadSync()).

mzumsande · 2022-07-21T17:45:09Z

ACK 11780f2

11780f2 doc: BaseIndex sync behavior with empty datadir (James O'Beirne) Pull request description: Make a note about a potentially confusing behavior with `BaseIndex::m_synced`; if the user starts bitcoind with an empty datadir and an index enabled, BaseIndex will consider itself synced (as a degenerate case). This affects how indices are built during IBD (relying solely on BlockConnected signals vs. using ThreadSync()). ACKs for top commit: mzumsande: ACK 11780f2 Tree-SHA512: 0b530379e57c62e05d2ddca7bb8e2c936786fa00678f9eaa1bb3742d957c48f141d46f936734b03f6673d964abc7eb72c1769f1784b9d3563d218e96296b7afd

DrahtBot added the UTXO Db and Indexes label Jul 18, 2021

Bocoi5011 reviewed Jul 18, 2021

View reviewed changes

bitcoin deleted a comment from Bocoi5011 Jul 19, 2021

Jaysonmald approved these changes Jul 19, 2021

View reviewed changes

DrahtBot mentioned this pull request Jul 19, 2021

Improve Indices on pruned nodes via prune blockers #21726

Merged

maflcko reviewed Jul 19, 2021

View reviewed changes

DrahtBot mentioned this pull request Jul 24, 2021

assumeutxo #15606

Closed

18 tasks

ryanofsky approved these changes Nov 2, 2021

View reviewed changes

jamesob closed this Nov 30, 2021

jamesob reopened this Jan 10, 2022

jamesob mentioned this pull request Jan 10, 2022

assumeutxo: indexing changes #24006

Closed

DrahtBot mentioned this pull request Feb 2, 2022

indexes: Stop using node internal types and locking cs_main, improve sync logic #24230

Draft

mzumsande reviewed Mar 20, 2022

View reviewed changes

src/index/base.cpp Outdated Show resolved Hide resolved

ryanofsky approved these changes Mar 21, 2022

View reviewed changes

fjahr reviewed Apr 17, 2022

View reviewed changes

src/index/base.cpp Outdated Show resolved Hide resolved

bitcoin deleted a comment from Rokon22 Apr 18, 2022

DrahtBot mentioned this pull request May 23, 2022

indexes: Read the locator's top block during init, allow interaction with reindex-chainstate #25193

Merged

jamesob force-pushed the 2021-07-index-doc branch from 2b48b03 to 59c8765 Compare July 18, 2022 19:25

jamesob changed the title ~~index: doc/log BaseIndex sync behavior with empty datadir~~ doc: BaseIndex sync behavior with empty datadir Jul 18, 2022

jamesob force-pushed the 2021-07-index-doc branch from 59c8765 to 4ba81ca Compare July 21, 2022 14:31

jamesob force-pushed the 2021-07-index-doc branch from 4ba81ca to 11780f2 Compare July 21, 2022 14:33

maflcko merged commit b8067cd into bitcoin:master Jul 21, 2022

bitcoin locked and limited conversation to collaborators Jul 21, 2023

doc: BaseIndex sync behavior with empty datadir #22485

doc: BaseIndex sync behavior with empty datadir #22485

Uh oh!

Conversation

jamesob commented Jul 18, 2021

Uh oh!

Bocoi5011 left a comment • edited by fanquake Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Jul 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

maflcko Jul 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamesob Jul 19, 2021

Choose a reason for hiding this comment

Uh oh!

ryanofsky Nov 2, 2021

Choose a reason for hiding this comment

Uh oh!

jamesob Jan 11, 2022

Choose a reason for hiding this comment

Uh oh!

ryanofsky Mar 21, 2022

Choose a reason for hiding this comment

Uh oh!

tryphe commented Jul 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jamesob commented Oct 29, 2021

Uh oh!

ryanofsky left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky Nov 2, 2021

Choose a reason for hiding this comment

Uh oh!

jamesob commented Jan 10, 2022

Uh oh!

maflcko commented Jan 11, 2022

Uh oh!

jamesob commented Jan 11, 2022

Uh oh!

mzumsande left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky Mar 21, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fjahr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mzumsande commented Jul 21, 2022

Uh oh!

Uh oh!

Bocoi5011 left a comment •

edited by fanquake

Loading

DrahtBot commented Jul 19, 2021 •

edited

Loading

maflcko Jul 19, 2021 •

edited

Loading

tryphe commented Jul 30, 2021 •

edited

Loading

ryanofsky left a comment •

edited

Loading