Change host API to separate fetching chain from fetching power tables #256
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #256 +/- ##
==========================================
- Coverage 71.66% 71.63% -0.04%
==========================================
Files 31 31
Lines 2333 2337 +4
==========================================
+ Hits 1672 1674 +2
- Misses 532 533 +1
- Partials 129 130 +1
|
| @@ -153,10 +156,14 @@ func (p *Participant) beginInstance() error { | |||
| if err := chain.Validate(); err != nil { | |||
| return fmt.Errorf("invalid canonical chain: %w", err) | |||
| } | |||
|
|
|||
| power, beacon, err := p.host.GetCommitteeForInstance(p.nextInstance) | |||
There was a problem hiding this comment.
In the next PR, these values will be cached in the participant and used to validate message. So each power table need only be computed once.
| // ReceiveDecision (or known to be final via some other channel). | ||
| GetCanonicalChain() (chain ECChain, power PowerTable, beacon []byte) | ||
| // Returns the chain to propose for a new GPBFT instance. | ||
| // This should be a suffix of the chain finalised by the immediately prior instance. |
There was a problem hiding this comment.
We probably want a stronger wording for chain suffix?
| // This should be a suffix of the chain finalised by the immediately prior instance. | |
| // This must be a suffix of the chain finalised by the immediately prior instance. |
Thinking out loud: It would also be nice to be able to validate this inside F3 but I am not sure how if F3 delegates storage of finalised chains to the host. As is it feels error prone with dire consequences in case of error.
There was a problem hiding this comment.
The comment is written from the narrow point of view of what does this code need. And in fact it will function correctly whether or not this is true (e.g. at genesis, or in case of a social agreement to re-start somewhere else, etc). This code doesn't need it to be a suffix, we can't write any test that will fail if it isn't, etc. Whether or not the multi-party protocol will make progress if this doesn't hold is a higher level matter.
I'd rather delete the comment than change it to a must.
gpbft/api.go
Outdated
| // or known to be final via some other channel (e.g. when bootstrapping the protocol). | ||
| // The offset (how many instances to look back) is determined by the host. | ||
| // Returns an error if the power table for the instance is not available. | ||
| GetCommitteeForInstance(instance uint64) (power PowerTable, beacon []byte, err error) |
There was a problem hiding this comment.
Thinking out loud: The phrase Committee seems reasonable. But, unless it is already used elsewhere, I am curious if we can avoid introducing a new terminology. Having said that, I am failing to think of an existing alternative.
There was a problem hiding this comment.
Yes it is new in the code (though common in the literature). If we generalised to sub-sampling the power table in the future (for scale), this would be exactly the right term.
I went through a similar thought process as you but couldn't come up with anything better.
…, in preparation for using the latter for validation of early messages.
|
Fuzz test failed on commit 6c21ef9. To troubleshoot locally, download the seed corpus using GitHub CLI by running: gh run download 9181466960 -n testdataAleternatively, download directly from here. |
Step 1 of #151. This is in preparation for fetching power tables for validation of messages for future instances.
See discussion in #151 about the interface design. The powertable offset parameter is encapsulated by the host, which can respond to a request for the power table to use for some (possibly-future) instance. This capability isn't used yet.
This change also removes fixed beacons in the sim, which was previously using the same beacon for every instance. Now it's generated (by setting it equal to the base tipset key bytes).