das light client (mvp) #323
Conversation
- clarify we need sequential verification - details on LightBlock changes
they are only used there anyways having exported method only for tests is meh
Codecov Report
@@ Coverage Diff @@
## master #323 +/- ##
==========================================
- Coverage 61.98% 61.79% -0.20%
==========================================
Files 263 262 -1
Lines 22931 22919 -12
==========================================
- Hits 14213 14162 -51
- Misses 7207 7258 +51
+ Partials 1511 1499 -12
|
oneliner shamelessly stolen from tendermint/tendermint#6202
| // persistent kvstore application and special consensus wal instance | ||
| // (byteBufferWAL) and waits until numBlocks are created. | ||
| // If the node fails to produce given numBlocks, it returns an error. | ||
| func WALGenerateNBlocks(t *testing.T, wr io.Writer, numBlocks int) (err error) { |
There was a problem hiding this comment.
These are only used in consensus/wal_test.go. Hence, I've moved them there (and it also helped with re-using the same ipfsAPI object in TestMain, see consensus/replay_test.go).
| // waitingTime estimates how long it should take for a node to reach the height. | ||
| // More nodes in a network implies we may expect a slower network and may have to wait longer. | ||
| func waitingTime(nodes int) time.Duration { | ||
| return time.Duration(30+(nodes*4)) * time.Second | ||
| } |
There was a problem hiding this comment.
slightly modified version of: tendermint/tendermint#6202
light/verifier.go
Outdated
| @@ -177,6 +178,14 @@ func ValidateTrustLevel(lvl tmmath.Fraction) error { | |||
| return nil | |||
| } | |||
|
|
|||
| func ValidateNumSamples(numSamples uint32) error { | |||
| maxShares := consts.MaxSquareSize * consts.MaxSquareSize | |||
| if numSamples == 0 && numSamples < uint32(maxShares) { | |||
There was a problem hiding this comment.
maxShares should never be 0, so the second condition is trivially true if the former is true. Should remove (and ideally place these assumptions in a separate file with static assertions like https://github.com/bitcoin-core/secp256k1/blob/3dc8c072b6d84845820c1483a2ee21094fb87cc3/src/assumptions.h).
Also, this should be numSamples > uint32(maxShares).
There was a problem hiding this comment.
Great find. Removed the 2nd condition as the code will calculate the actual number of samples as:
numSamples := min(c.numSamples, uint32(numRows*numRows))ideally place these assumptions in a separate file with static assertions like https://github.com/bitcoin-core/secp256k1/blob/3dc8c072b6d84845820c1483a2ee21094fb87cc3/src/assumptions.h).
What does static mean in this context? The other checks like this are in this file. I kept the new one here for consistency with the existing code.
There was a problem hiding this comment.
Static as in your program doesn't even compile without an error if these assumptions are violated. I don't know if Go has such a thing, but something like a small module that runs when the program starts that panics if certain fundamental assumptions are violated (e.g. the platform not being 64 bits, or having set a parameter to a value that was assumed impossible like setting the max size to 0) would serve that purpose. Either way it's orthogonal to this PR.
There was a problem hiding this comment.
Static as in your program doesn't even compile without an error if these assumptions are violated.
Yeah, that does not work here as the inputs that are checked are user input (not known during compile time).
There was a problem hiding this comment.
I don't know if Go has such a thing, but something like a small module that runs when the program starts
https://golang.org/doc/effective_go#init
but this is also during run time, not compile time.
There was a problem hiding this comment.
I meant that the max size parameter is not set to 0. That's a compile-time constant, and parts of the code assume it's > 0.
Crashing the program before it even does anything, deterministically, isn't great...but it's better than crashing in the middle randomly based on user input.
- ipfs node will also be shut down
| func ValidateNumSamples(numSamples uint32) error { | ||
| if numSamples == 0 { | ||
| return fmt.Errorf("number of samples must be > 0") | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
nit: are we planning on adding more checks in this function? perhaps it would be slightly simpler to just add this check to the place where it's needed?
There was a problem hiding this comment.
nit: are we planning on adding more checks in this function?
not sure yet.
perhaps it would be slightly simpler to just add this check to the place where it's needed?
I think the code is more consistent with the already existing code and I would argue that that place is more readable like this (alternatively, we could introduce a variable e.g. isValidNumSamples := numSamples != 0. That would the same effect).
Co-authored-by: Hlib Kanunnikov <hlibwondertan@gmail.com>
|
eeek, this was not meant to be merged as a merge commit 🤦🏼 Forgot to cleanup the commit history or squash merge instead. |
Description
MVP data availability sampling light client
For instructions how to run this: #323 (comment)
ref: #311