Skip to content

MAT-156 Span sync#1

Merged
jdkanani merged 6 commits intomasterfrom
span-sync
Nov 10, 2019
Merged

MAT-156 Span sync#1
jdkanani merged 6 commits intomasterfrom
span-sync

Conversation

@jdkanani
Copy link
Copy Markdown
Contributor

No description provided.

@linear
Copy link
Copy Markdown

linear bot commented Oct 29, 2019


// if current block is first block of last sprint in current span
h := header.Number.Uint64()
if span.EndBlock > c.config.Sprint && span.EndBlock-c.config.Sprint+1 == h {
Copy link
Copy Markdown
Contributor

@mankenavenkatesh mankenavenkatesh Nov 4, 2019

Choose a reason for hiding this comment

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

Should we check for ANY block of last sprint instead of only first block to be on safer side?
Incase during first blocK, if commitSpan throws error, we need to retry.

Copy link
Copy Markdown
Contributor Author

@jdkanani jdkanani Nov 5, 2019

Choose a reason for hiding this comment

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

Span fetch needs anchoring. It needs to be fetched at particular point, otherwise it will have problem while replaying the state. Currently, that point is first block of last sprint.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Got it.
But if checkAndCommitSpan returns error, we should not proceed with block finalisation as span and state is also not committed onto contract.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Edge cases need to be figured out. Example — figure out a way where no span proposed on heimdall.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Okay

recents, _ := lru.NewARC(inmemorySnapshots)
signatures, _ := lru.NewARC(inmemorySignatures)
vABI, _ := abi.JSON(strings.NewReader(validatorsetABI))
sABI, _ := abi.JSON(strings.NewReader(stateReceiverABI))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

handle error ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It won't throw error here since ABIs are hardcoded for now.

return nil, err
}
// get validators and current span
validators, _ := c.GetCurrentValidators(number, number+1)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is there a case where this might throw an error?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

}

// HeimdallSpan represents
type HeimdallSpan struct {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This naming convention is hard to understand, can we think of something better? Maybe more comments.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We will use Heimdall package directly.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Okay, resolving for now.

@vaibhavchellani
Copy link
Copy Markdown
Contributor

@jdkanani please also remove unwanted commented blocks of code.

Copy link
Copy Markdown

@0xAshish 0xAshish left a comment

Choose a reason for hiding this comment

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

Mech wise looks good to me @vaibhavchellani @mankenavenkatesh can review more regarding golang and bor wise things.


// ---

// MinimalVal is the minimal validator representation
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

good stuff we need these lightweight structs for p2p communications.

Copy link
Copy Markdown
Contributor

@vaibhavchellani vaibhavchellani left a comment

Choose a reason for hiding this comment

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

LGTM

@jdkanani jdkanani merged commit ae1b056 into master Nov 10, 2019
@jdkanani jdkanani deleted the span-sync branch November 10, 2019 13:27
jdkanani added a commit that referenced this pull request Nov 23, 2020
Bor related changes on updated geth
jdkanani added a commit that referenced this pull request Nov 22, 2021
pratikspatil024 pushed a commit that referenced this pull request Apr 28, 2025
…079)

This PR is #1 of a 3-part series that implements the new log index
intended to replace core/bloombits.
Replaces ethereum/go-ethereum#30370

This part implements the new data structure, the log index generator and
the search logic. This PR has most of the complexity but it does not
affect any existing code yet so maybe it is easier to review separately.

FilterMaps data structure explanation:
https://gist.github.com/zsfelfoldi/a60795f9da7ae6422f28c7a34e02a07e

Log index generator code overview:
https://gist.github.com/zsfelfoldi/97105dff0b1a4f5ed557924a24b9b9e7

Search pattern matcher code overview:
https://gist.github.com/zsfelfoldi/5981735641c956afb18065e84f8aff34

Note that the possibility of a tree hashing scheme and remote proof
protocol are mentioned in the documents above but they are not exactly
specified yet. These specs are WIP and will be finalized after the local
log indexer/filter code is finalized and merged.

---------

Co-authored-by: Felix Lange <fjl@twurst.com>
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.

4 participants