Skip to content
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

Move VRF proof from Header to Entropy for compatibity #559

Merged
merged 7 commits into from
Feb 9, 2023

Conversation

ulbqb
Copy link
Member

@ulbqb ulbqb commented Feb 6, 2023

Abstract

This PR move VRF proof and round from Header to new field, Entropy for Tendermint compatibity.

Background

Like #543 and #546, Ostracon should be as compatible as possible with IBC and Cosmos-SDK. However the compatibility is low for light client, since Ostracon's header structure is different from Tendermint's one. Therefore, the Ostracon header should be identical to the Tendermint.

Main Changes

message Block {
  Header          header          = 1 [(gogoproto.nullable) = false];
  Data            data            = 2 [(gogoproto.nullable) = false];
  EvidenceList    evidence        = 3 [(gogoproto.nullable) = false];
  Commit          last_commit     = 4;
  Entropy         entropy         = 5; // new field!!
}

// new message!!
message Entropy {
  int32 round = 1;
  bytes proof = 2;
}

message Header {
  ...
  // *** Ostracon Extended Fields ***
  // Note that MaxHeaderSize must be modified when adding/removing fields.

  // vrf info
  // int32 round = 1000; // remove this field!!
  // bytes proof = 1001; // remove this field!!
}

proof and round are removed from the header and added to a new field Entropy. This makes the ostracon header identical to the Tendermint. Removing the proof from the header has various effects. Therefore, there are the following considerations, and fixes have been made where necessary.

Considerations and Minor Changes

Can Entropy be trusted?

Yes. There is no VRF proof in Header. However, block id contains Markle root hash of the entire block and validators sign to block id, so it guarantees that Entropy is single at every height.

How is Entropy used securely?

// I want to use targetBlock.Entropy securely.
var targetBlock types.Block
// trustedBlockID is pre-verified.
var trustedBlockID types.BlockID

if !targetBlock.ValidateBasic() {
    panic("invalid block")
}
targetBlockID := types.BlockID{
    Hash: targetBlock.Hash(), 
    PartSetHeader: targetBlock.MakePartSet(types.BlockPartSizeBytes).Header(),
}
if !bytes.Equal(trustedBlockID.Hash, targetBlockID.Hash) {
    panic("invalid header")
}
// At this point, entropy is not used securely.
// It need the following validation.
if !trustedBlockID.PartSetHeader.Equals(targetBlockID.PartSetHeader) {
    panic("invalid entropy")
}

fmt.Printf("%v is secure entropy\n", targetBlock.Entropy)

BlockID.Hash is the Header hash. Also Header does not contain Entropy or Entropy hash. Therefore, validating BlockID.Hash is not enough to securely handle Entropy, and it is necessary to validate the Merkle root hash of the block. PartSetHeader validation is added, because PartSetHeader contains the Merkle root hash. This PR adds PartSetHeader validation at:

  • consensus.State.enterPrecommit
  • consensus.State.tryFinalizeCommit
  • consensus.State.handleCompleteProposal
  • consensus.State.AddVote
  • light.rpc.Client.Block

Is Entropy used in light clients?

Yes. However, since there is no Entropy or Entropy hash in the Header, light clients need to download the block to use Entropy securely. But light clients doesn't validate the proposer, so it doesn't matter.

Is Entropy used in ABCI apps?

message RequestBeginBlock {
  bytes                          hash                    = 1;
  tendermint.types.Header        header                  = 2 [(gogoproto.nullable) = false];
  tendermint.abci.LastCommitInfo last_commit_info        = 3 [(gogoproto.nullable) = false];
  repeated tendermint.abci.Evidence byzantine_validators = 4 [(gogoproto.nullable) = false];
  ostracon.types.Entropy            entropy              = 5 [(gogoproto.nullable) = false]; // new field!!
}

Yes. ABCI apps need to be able to use VRF proofs for validator election, however VRF proof has been removed from the Header. This PR adds Entropy to RequestBeginBlock in order to use Entropy in ABCI apps.

Is the statesync able to be executed?

Yes. The statesync is executed using the light client. Nodes need the LastProofHash to reconstruct the state, however it can't be got from the LightBlock. This PR makes it possible to get LastProofHash by getting block using rpc client.

Result

Header and commit are now identical to Tendermint. This theoretically allows us to use the Tendermint light client as is.

Closes: #554

@ulbqb ulbqb changed the title move VRF proof from Header to Entropy Move VRF proof from Header to Entropy Feb 6, 2023
@ulbqb ulbqb self-assigned this Feb 6, 2023
@codecov
Copy link

codecov bot commented Feb 6, 2023

Codecov Report

Merging #559 (3b702c3) into main (9d17c23) will decrease coverage by 0.15%.
The diff coverage is 71.53%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #559      +/-   ##
==========================================
- Coverage   66.23%   66.09%   -0.15%     
==========================================
  Files         278      278              
  Lines       36943    36999      +56     
==========================================
- Hits        24471    24456      -15     
- Misses      10713    10783      +70     
- Partials     1759     1760       +1     
Impacted Files Coverage Δ
evidence/pool.go 69.42% <ø> (ø)
evidence/reactor.go 61.07% <ø> (ø)
light/store/db/db.go 62.61% <ø> (ø)
statesync/stateprovider.go 31.20% <0.00%> (-1.39%) ⬇️
types/block_meta.go 72.34% <ø> (ø)
types/light.go 74.80% <ø> (ø)
state/execution.go 66.58% <57.14%> (-0.33%) ⬇️
types/evidence.go 59.60% <71.42%> (ø)
types/block.go 79.21% <79.45%> (-0.74%) ⬇️
consensus/state.go 72.40% <85.71%> (-0.05%) ⬇️
... and 14 more

@ulbqb ulbqb changed the title Move VRF proof from Header to Entropy Move VRF proof from Header to Entropy for compatibity Feb 6, 2023
@ulbqb ulbqb force-pushed the fix/move_vrf_proof branch from dfc7bad to f9e8761 Compare February 7, 2023 08:59
@ulbqb ulbqb requested a review from zemyblue February 7, 2023 09:00
@ulbqb ulbqb marked this pull request as ready for review February 7, 2023 09:00
@ulbqb ulbqb requested review from Kynea0b, torao and tnasu as code owners February 7, 2023 09:00
@torao torao added the C: enhancement Classification: New feature or its request, or improvement in maintainability of code label Feb 7, 2023
abci/example/kvstore/kvstore_test.go Outdated Show resolved Hide resolved
abci/types/messages_test.go Outdated Show resolved Hide resolved
evidence/pool.go Outdated Show resolved Hide resolved
evidence/reactor.go Outdated Show resolved Hide resolved
light/store/db/db.go Outdated Show resolved Hide resolved
p2p/conn/connection_test.go Outdated Show resolved Hide resolved
proto/ostracon/types/types.proto Outdated Show resolved Hide resolved
Copy link
Member

@zemyblue zemyblue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about using following rule about all import ordering?

  1. go default library like fmt.
  2. external library not line
  3. other line library, ex: github.com/line/tm-db
  4. ostracon itself library

proto/ostracon/types/block.proto Outdated Show resolved Hide resolved
proxy/mocks/app_conn_consensus.go Show resolved Hide resolved
abci/client/mocks/client.go Show resolved Hide resolved
proto/ostracon/abci/types.proto Outdated Show resolved Hide resolved
state/mocks/store.go Show resolved Hide resolved
statesync/stateprovider.go Show resolved Hide resolved
@ulbqb ulbqb requested review from tnasu and zemyblue February 8, 2023 05:22
consensus/state.go Show resolved Hide resolved
light/rpc/client.go Show resolved Hide resolved
@Kynea0b
Copy link
Contributor

Kynea0b commented Feb 8, 2023

I have a simple question. Please tell me why this file proto/ostracon/types/evidence.proto was deleted? Now that Ostracon and Tendemrint have compatible headers, does that mean that evidence.proto just for this Ostracon is no longer necessary?

@ulbqb
Copy link
Member Author

ulbqb commented Feb 8, 2023

I have a simple question. Please tell me why this file proto/ostracon/types/evidence.proto was deleted? Now that Ostracon and Tendemrint have compatible headers, does that mean that evidence.proto just for this Ostracon is no longer necessary?

@kokeshiM0chi

Yes. The evidence proto became the same as Tendermint. For compatibility, the Tendermint proto should be used as much as possible.

@ulbqb ulbqb requested a review from torao February 8, 2023 09:13
Copy link
Member

@zemyblue zemyblue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@Kynea0b Kynea0b left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ulbqb ulbqb merged commit 5aca894 into Finschia:main Feb 9, 2023
@ulbqb ulbqb deleted the fix/move_vrf_proof branch February 9, 2023 08:18
torao pushed a commit to torao/ostracon that referenced this pull request Feb 9, 2023
* move proof and round to entropy

* fix abci

* remove duplicated proto

* fix mocks

* fix import ordering

* fix proto number

* fix test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: enhancement Classification: New feature or its request, or improvement in maintainability of code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove VRF Proof from header for compatibility
5 participants