-
Notifications
You must be signed in to change notification settings - Fork 662
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
ACP-113 : Provable Virtual Machine Randomness #3143
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.
I think this needs some integration tests, specifically some with failing verification.
@@ -122,6 +123,8 @@ func (b *postForkOption) buildChild(ctx context.Context) (Block, error) { | |||
parentID, | |||
b.Timestamp(), | |||
parentPChainHeight, | |||
[]byte{}, // TODO : verify me. |
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.
Please provide more context in a TODO in case it's not you that picks it up in the future.
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.
sorry for not being more clear here. @StephenButtolph - do we need to have a VRF Sig on this block type ?
the "verify me" was meant to be read as "before merging this in, we need to decide if we need VRF Sig here at all. Either case, more work here is needed."
vms/proposervm/pre_fork_block.go
Outdated
@@ -236,6 +237,7 @@ func (b *preForkBlock) buildChild(ctx context.Context) (Block, error) { | |||
newTimestamp, | |||
pChainHeight, | |||
innerBlock.Bytes(), | |||
[]byte{}, // TODO : verify me |
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.
As above.
vms/proposervm/post_fork_block.go
Outdated
|
||
var proposerPublicKey *bls.PublicKey | ||
// if there is a known proposer. ( i.e. block was signed ) | ||
if childProposer != ids.EmptyNodeID { |
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.
When in this part of the code it's difficult to understand what happens when the the pubkey is nil*. I think it would help if you (a) renamed block.VerifySignature()
to VerifyVRFSignatureOrHash()
and (b) put a comment at the declaration of proposerPublicKey
that says something like "if this remains nil then VRF verification will assume a hash-based output".
*In fact, I'd originally written a comment here asking if we should be returning an error, despite having thoroughly reviewed the verification implementation 🤦
vms/proposervm/config.go
Outdated
StakingBLSKey *bls.SecretKey | ||
|
||
// VRFSig activation time | ||
VRFSigTime time.Time |
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.
The config is specified externally (in the node package)
vms/proposervm/block/block.go
Outdated
VRFSig() []byte | ||
|
||
// VerifySignature validates the correctness of the VRF signature. | ||
VerifySignature(*bls.PublicKey, []byte, ids.ID, uint32) bool |
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.
nit: might be good to name these parameters so we don't need to look at an implementation to see what they are.
i.e.
(pk *bls.PublicKey, parentVRFSig []byte, chainID ids.ID, networkID uint32)
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.
np
vms/proposervm/block.go
Outdated
@@ -38,6 +38,8 @@ var ( | |||
errProposerMismatch = errors.New("proposer mismatch") | |||
errProposersNotActivated = errors.New("proposers haven't been activated yet") | |||
errPChainHeightTooLow = errors.New("block P-chain height is too low") | |||
errInvalidVRFSignature = errors.New("invalid VRF signature") | |||
errVRFSignaturePresents = errors.New("unexpected VRF signature presents") |
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.
wording nit. I think just changing presents
to present
would be ok too.
errVRFSignaturePresents = errors.New("unexpected VRF signature presents") | |
errUnexpectedVRFSignature = errors.New("unexpected VRF signature present") |
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.
done
vms/proposervm/block/block.go
Outdated
VRFSig []byte `serialize:"true"` | ||
} | ||
|
||
type unsignedBlockTypes interface { |
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.
See Dhruba's point above about the codec's versioning. Also, I think we could instead declare this interface as:
type unsignedBlockTypes interface {
ParentID() ids.ID
Block() []byte
VRFSig() []byte
PChainHeight() uint64
innerCertificate() []byte
innerTimestamp() int64
}
And then implement that interface for statelessUnsignedBlock
and statelessUnsignedBlockV0
so that below, we don't need to switch
on the type and we can do, for example:
func (b *statelessBlock[T]) ParentID() ids.ID {
return b.StatelessBlock.ParentID()
}
without using a switch.
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.
the versioned codec eliminates the needs for that..
vms/proposervm/block/build.go
Outdated
// | networkID: | uint32 | 4 bytes | | ||
// +-----------------------+----------+------------+ | ||
|
||
buffer := make([]byte, len(vrfRngRootPrefix)+ids.IDLen+4) |
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.
nit: could use wrappers.IntLen
instead of 4
to remove use of a "magic number" even though it's fairly obvious from the comments here
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.
done.
vms/proposervm/block/build.go
Outdated
buffer := make([]byte, len(vrfRngRootPrefix)+ids.IDLen+4) | ||
copy(buffer, vrfRngRootPrefix) | ||
copy(buffer[len(vrfRngRootPrefix):], chainID[:]) | ||
binary.LittleEndian.PutUint32(buffer[len(vrfRngRootPrefix)+ids.IDLen:], networkID) |
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.
Any reason in particular we use LittleEndian
here? We use BigEndian
everywhere else in the codecase, I think.
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.
done.
vms/proposervm/block/build.go
Outdated
// signature only if it presents. Otherwise, we'll keep it empty. | ||
if len(parentBlockVRFSig) == 0 { | ||
// no parent block signature. | ||
return []byte{} |
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.
Nit: do we need to return []byte{}
instead of nil
? Typically I think we return the latter, unless []byte{}
has a special meaning distinct from 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.
done
vms/proposervm/block/build.go
Outdated
var err error | ||
|
||
var blkBytes []byte |
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.
nit: is there a reason we declare these here instead of with :=
like this?
blkBytes, err := marshalBlock(block)
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.
done
Co-authored-by: Stephen Buttolph <stephen@avalabs.org>
vms/proposervm/block/block.go
Outdated
// The following ensures that we would initialize the vrfSig member only when | ||
// the provided signature is 96 bytes long. That supports both statelessUnsignedBlockV0 & statelessUnsignedBlock | ||
// variations, as well as optional 32-byte hashes stored in the VRFSig. | ||
if len(b.StatelessBlock.VRFSig) == bls.SignatureLen { |
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.
Should len(b.StatelessBlock.VRFSig)
always be either 0
or bls.SignatureLen
? If so, should we assert that here?
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.
when a proposer doesn't have a BLS key, they would hash the previous signature instead of signing it. In that case, the VRFSig would be 32 byte long.
I think that adding another case here that validates that the signature length is in [0,32,96] makes sense here. I'll add that.
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.
done
vms/proposervm/block/block.go
Outdated
return b.timestamp | ||
} | ||
|
||
func (b *statelessBlock) Proposer() ids.NodeID { |
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.
Nit: we could reduce the diff in this PR by keeping these methods in their original place but idc if they move
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.
done.
vms/avm/vm.go
Outdated
@@ -395,7 +395,7 @@ func (vm *VM) GetBlockIDAtHeight(_ context.Context, height uint64) (ids.ID, erro | |||
*/ | |||
|
|||
func (vm *VM) Linearize(ctx context.Context, stopVertexID ids.ID, toEngine chan<- common.Message) error { | |||
time := vm.Config.Upgrades.CortinaTime | |||
time := vm.Upgrades.CortinaTime |
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 seems like a change unrelated to this PR - mind reducing the diff?
time := vm.Upgrades.CortinaTime | |
time := vm.Config.Upgrades.CortinaTime |
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.
done.
if shouldBuildSignedBlock { | ||
childBlockVrfSig = block.NextBlockVRFSig(parentBlockVRFSig, p.vm.Config.StakingBLSKey, p.vm.ctx.ChainID, p.vm.ctx.NetworkID) |
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.
Does this correctly differentiate between nodes that have registered their BLS key and those that haven't registered their BLS key?
Every node will have a non-nil StakingBLSKey
, but not all nodes have registered that key on the P-chain (which will be needed during verification of this block)
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.
I see. I believe that my code already handles the validation part correctly ( since it gets the BLS public key from the ValidatorState.GetValidatorSet
).
For the block building, would it work if I were to retrieve the validator set from ValidatorState.GetValidatorSet
and compare the public registered BLS key there with the block builder's public key ?
@@ -218,10 +222,23 @@ func (p *postForkCommonComponents) buildChild( | |||
return nil, err | |||
} | |||
|
|||
var childBlockVrfSig []byte |
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.
I don't love naming this a Sig
because it isn't always a signature.
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.
Would it make sense to break this out into some smaller PRs? Perhaps first a PR just defining the new serialization format of the blocks?
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.
I'll try to extract the changes to proposervm/block
onto it's own PR; but the context needs to remain the same.
vms/proposervm/block/build.go
Outdated
// | vrfSig : | [96]byte | 96 bytes | | ||
// +-------------------------+----------+------------+ | ||
if len(vrfSig) != bls.SignatureLen { | ||
return [32]byte{0} |
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.
return [32]byte{0} | |
return [32]byte{} |
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.
done
lc.RegisterType(&statelessBlock{}), | ||
lc.RegisterType(&option{}), | ||
Codec.RegisterCodec(CodecVersionV0, lcV0), | ||
Codec.RegisterCodec(CodecVersion, lc), |
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.
With the addition of this new codec version - where do we enforce that this codec version is not used until after the network upgrade?
vms/proposervm/block/build.go
Outdated
|
||
unsignedBytesWithEmptySignature, err := Codec.Marshal(CodecVersion, &blockIntf) | ||
var err error |
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 can be removed afaict
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.
done
vms/proposervm/block/build.go
Outdated
// The following ensures that we would initialize the vrfSig member only when | ||
// the provided signature is 96 bytes long. That supports both statelessUnsignedBlockV0 & statelessUnsignedBlock | ||
// variations, as well as optional 32-byte hashes stored in the VRFSig. | ||
if len(blockVrfSig) == bls.SignatureLen { | ||
block.vrfSig, err = bls.SignatureFromBytes(blockVrfSig) | ||
if err != nil { | ||
return nil, fmt.Errorf("%w: %w", errFailedToParseVRFSignature, 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.
Since this doesn't depend on the serialized format - would it make more sense to move this up into the initial construction of the block
?
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.
sure. done.
|
||
// temporary, set the bytes to the marshaled content of the block. | ||
// this doesn't include the signature ( yet ) | ||
blkBytes, err := marshalBlock(block) |
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.
Can we rename this like it was previously explicitly stating that these bytes are unsigned? I found this change pretty confusing when looking at the code.
@@ -151,6 +151,7 @@ message Handler { | |||
|
|||
message BuildBlockRequest { | |||
optional uint64 p_chain_height = 1; | |||
optional bytes randomness_seed = 2; |
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.
Does specifying optional do anything here? Or is this just for consistency with p_chain_height
?
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.
I used "optional" so that the serialized requests would remain backward compatible. Isn't that the case ?
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.
It isn't clear to me that optional bytes
is different from just bytes
. See the proto spec: https://protobuf.dev/programming-guides/proto3/#field-labels
Why this should be merged
This is the implementatinon of ACP-113, as described here.
How this works
(taken from the design doc)
Existing avalanche protocol breaks the block building into two parts : external and internal. The external block is the SnowMan++ block, whereas the internal block is the actual virtual machine block.
To support randomness, a BLS based VRF implementation is used, that would be recursively signing its own signatures as its message. Since the BLS signatures are deterministic, they provide a great way to construct a reliable VRF.
For proposers that do not have a BLS key associated with their node, the hash of the signature from the previous round.
In order to bootstrap the signatures chain, a missing signature would be replaced with a byte slice that is the hash product of a verifiable and trustable seed.
The changes proposed here would affect the way a block is being validated. Therefore, when this change gets implemented, it needs to be deployed as a mandatory upgrade.
How this was tested