-
Notifications
You must be signed in to change notification settings - Fork 108
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
State syncer mempool syncing and test suite updates #1442
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks great!
lib/db_utils.go
Outdated
// We check if we've already read this key and stored it in the cache. | ||
// Otherwise, we fetch the current value of this record from the DB. | ||
ancestralValue, getError = DBGetWithTxn(txn, snap, key) | ||
if isState || (isCoreState && eventManager != nil && eventManager.isMempoolManager) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if isState || (isCoreState && eventManager != nil && eventManager.isMempoolManager) { | |
if isState || (eventManager != nil && eventManager.isMempoolManager && isCoreStateKey(key)) { |
This is very minor, but we can avoid the extra call to isCoreStateKey
if we reorder this and check the event manager first.
Also can we add a comment about why we check for isCoreState
here? something like If this is a coreState prefix and running a state syncer that is syncing mempool/uncommitted state, we need to get the last committed value for this badger key
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addressed in #1447 (comment/documentation still should be addressed).
// Decode the ancestral record. | ||
ancestralRecord, err := DecodeStateKey(stateChangeEntry.KeyBytes, stateChangeEntry.AncestralRecordBytes) | ||
if err != nil { | ||
glog.Fatalf("Server._handleStateSyncerOperation: Error decoding ancestral record: %v", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fatal is going to kill the node if we hit this error. Are we sure that's what we want?
stateChangeEntry.AncestralRecord = ancestralRecord | ||
stateChangeEntry.AncestralRecordBytes = nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we set AncestralRecordBytes
to nil?
@@ -899,77 +911,45 @@ func (stateChangeSyncer *StateChangeSyncer) SyncMempoolToStateSyncer(server *Ser | |||
startTime = time.Now() | |||
glog.V(2).Infof("Mempool synced len after flush: %d", len(stateChangeSyncer.MempoolSyncedKeyValueMap)) | |||
|
|||
//Check to see if every txn hash in our cached txns is in the first n txns in the mempool. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love that we're getting rid of this cache in here.
…testing-suite address feedback from LN on z/testing-suite branch
No description provided.