[Storehouse] 004 - Payloadless Remote ledger service - #8611
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
939e60e to
d113e4c
Compare
f5eaeef to
890ed2b
Compare
| HasState(state State) bool | ||
| // | ||
| // No error returns are expected during normal operation. | ||
| HasState(state State) (bool, error) |
There was a problem hiding this comment.
includes a breaking change: HasState now returns a (bool, error), instead of bool. This means this version would require the ledger service to be restarted with compatible version. Be careful during HCU
| // Mode check is a configuration-correctness gate. If we | ||
| // connected to a payloadless server while expecting full, | ||
| // crash now rather than fail later on every Get call. | ||
| verifyServerMode(c.ctx, c.infoClient, c.callTimeout, ledgerpb.LedgerMode_LEDGER_MODE_FULL, c.logger) |
There was a problem hiding this comment.
should't ServerInfo be called before InitialState. otherwise Initial state might already error because of the incorrect ledger mode, no?
There was a problem hiding this comment.
I think it's ok either way, because both payloadless ledger service and full ledger service support the InitialState call with the same signature. The current approach is to ensure the service has the state and then check if the right service is started.
| // [ledgerpb.LedgerMode_LEDGER_MODE_PAYLOADLESS]; passing UNSPECIFIED produces | ||
| // a server that reports UNSPECIFIED, which clients will treat as a | ||
| // misconfigured server and refuse to use. | ||
| func NewInfoService(mode ledgerpb.LedgerMode) *InfoService { |
There was a problem hiding this comment.
Is this ever called?
There was a problem hiding this comment.
Good catch, I didn't add this function to registration for service discovery.
| } | ||
| interims[i] = interim | ||
| } | ||
| pInst.Interims = interims |
There was a problem hiding this comment.
do we need to check that rest is empty here?
| bp, err := decodePayloadlessTrieBatchProof(rest) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error decoding payloadless batch proof: %w", err) | ||
| } |
There was a problem hiding this comment.
same: do we need to check that rest is empty?
| // A gRPC failure is surfaced to the caller rather than collapsed into a false | ||
| // return: false must mean "state genuinely absent", not "the server was | ||
| // unreachable", otherwise callers (e.g. execution state) would misreport a | ||
| // reachable state as pruned. | ||
| // | ||
| // No error returns are expected during normal operation. |
There was a problem hiding this comment.
Can this both be true: "gRPC failure is surfaced" and "No error returns are expected during normal operation"
| @@ -0,0 +1 @@ | |||
| package ledger | |||
2 participants
reopen for #8575
This PR includes a breaking change: HasState now returns a (bool, error), instead of bool. This means this version would require the ledger service to be restarted. Be careful during HCU