-
Notifications
You must be signed in to change notification settings - Fork 14
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 table verification #169
base: master
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.
Couple of questions regarding the newly added elements in the Output structs, otherwise looks good!
cmd/export_ledger_entry_changes.go
Outdated
} | ||
} | ||
} | ||
|
||
for checkpointLedgers := range verifyOutputs { | ||
_, err := verify.VerifyState(ctx, verifyOutputs[checkpointLedgers], archive, checkpointLedgers, verifyBatchSize) |
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.
Do you know where the verifyBatchSize
comes from? Do we know that 50000 is big enough?
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.
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.
This value can be changed. It's basically as big as possible but small enough to not cause OOM crash.
internal/utils/verify/verify.go
Outdated
return false, errors.Wrap(err, "addLiquidityPoolsToStateVerifier failed") | ||
} | ||
|
||
return true, 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.
There should be Verify()
call after Write()
'ing all ledger entries.
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.
@bartekn calling Verify()
against StateVerifier.currentEntries
makes sense when these entries come from the Captive Core? I'm struggling to implement this method as it is because the logic I used in this file is different than the original one.
If implementing this is really necessary, could you shed some light on how to?
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.
This method needs one argument which is the total number of ledger entries in the user's storage (in our case: BigQuery). This is necessary because due to a bug users can have extra entries. If the provided number is equal to the total number streamed state is correct, if not it means you have extra entries. Obviously you can fool this by passing the number of entries you got from state verifier but it's better to count all entries in the storage.
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.
Looks good and ready for release
Overview
This implements Horizon's
verify
package tostellar-etl
commandexport_ledger_entry_changes
. It only verifies entries from checkpoint ledgers (every 64 ledgers, every ~5 minutes).Changes
Raw*
fields typesAccountOutput
,OfferOutput
,PoolOutput
,ClaimableBalanceOutput
andTrustlineOutput
LedgerIsCheckpoint()
function toutils
packageTransformedOutput
type tointernal/transform/schema.go
to assist the Horizon'sverify
package implementationinternal/utils/verify.go
to work withstellar-etl
Closes GH#279
Closes GH#280
Closes GH#281