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

kernel pos index #3228

Closed
wants to merge 11 commits into from
Closed

Conversation

antiochp
Copy link
Member

@antiochp antiochp commented Feb 10, 2020

Maintain index of "recent" kernels so we can quickly lookup a kernel MMR pos (and block height) by kernel excess commitment.

This is similar to the existing output_pos index.
Important difference is outputs can be removed and pruned (after being spent), kernels are never removed. But we only want to maintain an index of "recent" kernels sufficient for the proposed NSKR relative kernel locks.

Compact the kernel pos index when we compact the txhashset. We maintain 2 weeks of kernel history in the index. This covers 7 days of full block history back to the horizon (for rewind purposes) and a further 7 days of kernel history to cover the max NSKR relative kernel lock period.

Populate the index as we go when calling apply_kernel() during processing of new full blocks.

Add test coverage around chain compaction and interaction with the kernel pos index.

Track an "undo list" for each full block so we can rewind the kernel_pos index as required when rewinding a block.


We do not need to track the kernel_pos index currently as it is unused.
But we need this implemented for efficient handling of NRD kernel lookups.
Once we have the NRD kernel feature variant we can modify the kernel_pos index to only index recent NRD kernels (currently we index all recent kernels).


Related #3273.

If we track kernel_pos via the "linked list" impl then we no longer need to maintain the "undo list" per block.
We can simply pop entries off the head of the list during rewind, rewinding each kernel in the block, leaving the index in the previous state in the case of duplicates.

@antiochp antiochp marked this pull request as ready for review February 13, 2020 11:03
@antiochp antiochp changed the title [WIP] kernel pos index kernel pos index Feb 13, 2020
@antiochp antiochp added this to the 3.1.0 milestone Feb 13, 2020
Copy link
Member

@quentinlesceller quentinlesceller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good to me 👍 Few small comments below.

chain/src/txhashset/txhashset.rs Outdated Show resolved Hide resolved
core/src/global.rs Show resolved Hide resolved
chain/src/chain.rs Outdated Show resolved Hide resolved
debug!(
"init_kernel_pos_index: {} kernels, took {}s",
kernel_count,
now.elapsed().as_secs()
Copy link
Member

@quentinlesceller quentinlesceller Feb 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be interesting to see if there is any difference with the comment above (without looping on an inclusive range).

.kernel_pos_iter()?
.filter(|(_, (_, height))| *height < cutoff_height)
.map(|(key, _)| batch.delete(&key))
.count();
Copy link
Member

@quentinlesceller quentinlesceller Feb 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiosity I'm wondering if there is a more efficient way to do that. Like some kind of filter_map or for_each.
Also we are loosing the error of the batch delete without any logging (I know it's actually the same situation for the output_pos)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean in terms of "efficient"?
We need to iterate over the full set of entries in the db.

@antiochp antiochp changed the title kernel pos index [DNM] kernel pos index Feb 14, 2020
@antiochp
Copy link
Member Author

antiochp commented Feb 14, 2020

Flagging as [DNM] for now - discussions under way around potentially maintaining the kernel_pos index purely in memory.

Edit: We do not want to maintain this purely in memory. We actually want transactional (lmdb database) semantics to allow efficient "rewind" via the per block kernel "undo list".

@quentinlesceller quentinlesceller removed this from the 3.1.0 milestone Feb 18, 2020
@antiochp antiochp force-pushed the kernel_pos_index branch 2 times, most recently from 25105bf to 531506c Compare March 4, 2020 16:34
@antiochp antiochp changed the title [DNM] kernel pos index kernel pos index Mar 21, 2020
@antiochp
Copy link
Member Author

Closing, replaced with #3273 (which is a more general solution).

@antiochp antiochp closed this Apr 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants