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

Block input bitmap rework #3236

Merged
merged 9 commits into from Feb 24, 2020
Merged

Conversation

@antiochp
Copy link
Member

antiochp commented Feb 21, 2020

Resolves #3235.
Extracted the "undo list" concept from the kernel index exploration (#3228) and took advantage of it for outputs (where undo == unspend during rewind).

This allows the output_pos index (quick lookup of output pos by output commitment) to more closely track the current utxo set in a transactional manner.
We now keep the output_pos index updated when applying new blocks and when rewinding existing blocks under a fork scenario.

This eliminates the edge-case of needing to account for "false positive" results in the index.
We still do not treat the output_pos as authoritative but there are currently believed to be no cases where it will temporarily diverge from the set of unspent output pos.

We rebuild (via optimized init) the output_pos_index on -

  1. startup, to ensure the node starts up in a consistent state
  2. compact (once outputs are removed and pruned/compacted we no longer need to keep them in the index)
  3. during fast sync, after receiving txhashset from a peer

We log discrepencies between the index and set of outputs during index rebuild.
This should give us some insight into whether our assumptions are correct here.
If we are comfortable that we can begin treating this index as authoritative then we will only need to rebuild the index on sync (3) above.

This PR is a relatively large change in terms of code changes. But conceptually it is pretty straightforward.

  • Update output_pos index when we call apply_block()
    • add new index entries for new outputs
    • remove index entries for spent outputs
  • Update output_pos on rewind (per block)
    • remove "future" outputs that no longer exist
    • re-add "unspent" entries to the index based on per-block "spent_index" (aka undo list)
  • Store a spent_index per block when we store the associated block in the db

We used to do something similar but not exactly the same -

  • we stored a bitmap of spent pos per block
    • this allowed us to rewind the utxo set itself, but not the associated index
  • we did not undo these in the index itself when rewinding
    • this led to false positives when reading the index, so we always went to the PMMR to corroborate what the index said (if index was inconsistent with PMMR then treat as spent)

The majority of the code change involves breaking the rewind functionality out to do it per iteratively block. Before we could simply take the union of all the per-block bitmaps. This has been reworked to rewind block by block to take advantage of our ability to rewind the index itself.


We were originally hoping to get the kernel_index impl into 3.1.0 but this was not possible given the timing of the 3.1.0 release. This PR gives us a way of confirming the approach works on the existing output_pos index.

@antiochp antiochp added this to the 3.1.0 milestone Feb 21, 2020
@antiochp antiochp self-assigned this Feb 21, 2020
@antiochp

This comment has been minimized.

Copy link
Member Author

antiochp commented Feb 21, 2020

Will rebase once #3233 has been merged to fix the croaring issue.

antiochp added 7 commits Feb 16, 2020
and reworking rewind to simply iterate over blocks, rewinding each incrementally
just use the order of the inputs in the block
@antiochp antiochp force-pushed the antiochp:block_input_bitmap_rework branch from 73e13fc to 00edeaa Feb 21, 2020
chain/src/types.rs Outdated Show resolved Hide resolved
chain/src/pipe.rs Show resolved Hide resolved
chain/src/txhashset/txhashset.rs Show resolved Hide resolved
@jaspervdm

This comment has been minimized.

Copy link
Member

jaspervdm commented Feb 21, 2020

I think these changes behave as expected, however I am not an expert in this area of the code. Would appreciate it if we have one additional review

@antiochp

This comment has been minimized.

Copy link
Member Author

antiochp commented Feb 24, 2020

Making an executive decision to merge this now. Feel free to continue review post merge.
I'd like to get the initial 3.1.0-beta tagged today.

@antiochp antiochp merged commit cb2b909 into mimblewimble:master Feb 24, 2020
10 checks passed
10 checks passed
mimblewimble.grin Build #20200222.2 succeeded
Details
mimblewimble.grin (linux api/util/store) linux api/util/store succeeded
Details
mimblewimble.grin (linux chain/core/keychain) linux chain/core/keychain succeeded
Details
mimblewimble.grin (linux pool/p2p/src) linux pool/p2p/src succeeded
Details
mimblewimble.grin (linux release) linux release succeeded
Details
mimblewimble.grin (linux servers) linux servers succeeded
Details
mimblewimble.grin (macos release) macos release succeeded
Details
mimblewimble.grin (macos test) macos test succeeded
Details
mimblewimble.grin (windows release) windows release succeeded
Details
mimblewimble.grin (windows test) windows test succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants
You can’t perform that action at this time.