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

MARF Trie identifier refactoring #1604

Merged
merged 12 commits into from
Jun 15, 2020
Merged

MARF Trie identifier refactoring #1604

merged 12 commits into from
Jun 15, 2020

Conversation

kantai
Copy link
Member

@kantai kantai commented May 19, 2020

This PR mostly does two things:

  1. Makes MARFs generic in the "trie identifier": allowing the burndb to use BurnchainHeaderHash directly with the MARF, rather than casting it into a BlockHeaderHash, and enabling the introduction of a new type StacksBlockId
  2. Introduces a StacksBlockId type for capturing the index block hash. Everywhere where an index hash was previously used now uses a StacksBlockId (a lot of code changes!)

Those changes are are almost exclusively type refactors -- mostly no actually functionality has changed.

There was one instance, however, where a change was made -- in relay.rs, the MicroblocksData.index_anchor_block field was previously being filled with the anchored block hash, rather than the index block hash.

@kantai kantai linked an issue May 19, 2020 that may be closed by this pull request
@jcnelson jcnelson requested review from lgalabru and jcnelson and removed request for lgalabru May 19, 2020 18:40
@kantai kantai requested a review from lgalabru May 19, 2020 19:12
Copy link
Contributor

@lgalabru lgalabru left a comment

Choose a reason for hiding this comment

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

Looks good to me, thank you!

@@ -1397,7 +1398,7 @@ impl HttpRequestType {
.ok_or(net_error::DeserializeError("Failed to match path to microblock minimum sequence group".to_string()))?
.as_str();

let block_hash = BlockHeaderHash::from_hex(block_hash_str)
let block_hash = StacksBlockId::from_hex(block_hash_str)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we also rename this block_hash instances?

@@ -675,16 +676,17 @@ impl Relayer {
Ok(_) => {
if need_relay {
// we didn't have this block before, so relay it.
// Group by anchored block hash, so we can convert them into
// Group by index block hash, so we can convert them into
Copy link
Contributor

Choose a reason for hiding this comment

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

Ha, isn’t “anchored block id” more accurate?

Copy link
Member

@jcnelson jcnelson left a comment

Choose a reason for hiding this comment

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

This LGTM, but I did have one lingering question -- besides unit tests, what's the point of keeping BlockHeaderHash around at this point?

@kantai
Copy link
Member Author

kantai commented Jun 15, 2020

This LGTM, but I did have one lingering question -- besides unit tests, what's the point of keeping BlockHeaderHash around at this point?

It only implements the MarfTrieIdentifier trait for unit tests (and is marked cfg(test)) -- I think it makes sense to leave that as is, since I think it's better to avoid changing old unit tests if possible.

@jcnelson
Copy link
Member

Is there a difference between BlockHeaderHash-as-used-as-the-index-hash versus BlockHeaderHash-as-used-as-the-Stacks-block-hash? I can see that I'll need to replace the former usage with StacksBlockId in my PRs when this merges, but what about the latter?

@kantai
Copy link
Member Author

kantai commented Jun 15, 2020

I can see that I'll need to replace the former usage with StacksBlockId in my PRs when this merges, but what about the latter?

The latter isn't changed -- BlockHeaderHash is the type used for the hash of the Stacks block header (probably StacksHeaderHash would be the better name)

@kantai kantai merged commit fc55700 into master Jun 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Chainstate: Index block hash should be its own type
3 participants