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.

Sign up for GitHub

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

Mandatory commitments only #1138

Closed
wants to merge 7 commits into from
Closed

Mandatory commitments only #1138

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
01ace02
identify and reject non mandatory commitments
alistair-singh Jan 31, 2024
428abe8
check the right block
alistair-singh Jan 31, 2024
4826c8e
add mandator flag
alistair-singh Jan 31, 2024
0211cb1
remove skip check
alistair-singh Jan 31, 2024
da89c96
remove checks
alistair-singh Jan 31, 2024
a791b2a
add config setting
alistair-singh Feb 6, 2024
d18ebc2
unused substitution
alistair-singh Feb 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions relayer/relays/beefy/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ type SourceConfig struct {
FastForwardDepth uint64 `mapstructure:"fast-forward-depth"`
// Period to sample the beefy updates (in number of blocks)
UpdatePeriod uint64 `mapstructure:"update-period"`
// Only submit mandatory commitments. The first commitment for each session which validators must sign.
DiscardNonMandatoryCommitments bool `mapstructure:"discard-non-mandatory-commitments"`
}

type SinkConfig struct {
Expand Down
6 changes: 6 additions & 0 deletions relayer/relays/beefy/polkadot-listener.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,9 +99,15 @@ func (li *PolkadotListener) scanCommitments(
},
"validatorSetID": currentValidatorSet,
"IsHandover": validatorSetID == currentValidatorSet+1,
"IsMandatory": result.IsMandatory,
"lastSyncedBeefyBlock": lastSyncedBeefyBlock,
})

if li.config.DiscardNonMandatoryCommitments && !result.IsMandatory {
logEntry.Info("Discarded due to not being a mandatory commitment.")
continue
}

validators, err := li.queryBeefyAuthorities(result.BlockHash)
if err != nil {
return fmt.Errorf("fetch beefy authorities at block %v: %w", result.BlockHash, err)
Expand Down
28 changes: 26 additions & 2 deletions relayer/relays/beefy/scanner.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package beefy

import (
"context"
"encoding/binary"
"fmt"
"time"

Expand Down Expand Up @@ -81,6 +82,7 @@ type ScanCommitmentsResult struct {
BlockNumber uint64
BlockHash types.Hash
Depth uint64
IsMandatory bool
Error error
}

Expand Down Expand Up @@ -129,6 +131,21 @@ func scanCommitments(ctx context.Context, api *gsrpc.SubstrateAPI, startBlock ui
return
}

isMandatory := false
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would rather we go all the way and add a documented MandatoryCommitmentsOnly config setting. Otherwise this code is going to be forgotten about it.

Copy link
Contributor

@yrong yrong Feb 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

We may not really need another config as it can be already achieved by setting

snowbridge/web/packages/test/config/beefy-relay.json

Line 8 in aab91ae

"update-period": 0
with a value big enough(i.e. more than blocks in an epoch).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in a791b2a.

We can use the update-period but this is more explicit and is good for testing the mmr fix as well. Currently turning this setting on will fail untill we update from upstream polkadot-sdk. I tested this change with cherry-picking .

$ git cherry-pick e5bb11b008ca6c1b5a07cab94a2327538f3bf15c

I think this setting also works well with incentivizing beefy relayers. For instance, our relayer can run in a mode where it only sync's mandatory commitments (i.e. 1 commitment per epoch). And if there are pending messages and an incentive is provided by a user to speed up the transaction then it can sync the next commitment it sees.

for _, digest := range block.Block.Header.Digest {
if !digest.IsConsensus {
continue
}
consensus := digest.AsConsensus
// Mandatory commitments contain a BEEF digest of type ConsensusLog::AuthoritiesChange (0x01)
// which signifies the the change of session authorities.
// https://github.com/paritytech/polkadot-sdk/blob/6a168ad57ad13ea0896f7684120f4fa15bfef474/substrate/primitives/consensus/beefy/src/lib.rs#L254C2-L254C19
if decodeEngineId(uint32(consensus.ConsensusEngineID)) == "BEEF" && consensus.Bytes[0] == 0x01 {
isMandatory = true
break
}
}

var commitment *types.SignedCommitment
for j := range block.Justifications {
sc := types.OptionalSignedCommitment{}
Expand Down Expand Up @@ -156,7 +173,7 @@ func scanCommitments(ctx context.Context, api *gsrpc.SubstrateAPI, startBlock ui
select {
case <-ctx.Done():
return
case out <- ScanCommitmentsResult{BlockNumber: result.BlockNumber, BlockHash: result.BlockHash, SignedCommitment: *commitment, Depth: result.Depth}:
case out <- ScanCommitmentsResult{BlockNumber: result.BlockNumber, BlockHash: result.BlockHash, SignedCommitment: *commitment, Depth: result.Depth, IsMandatory: isMandatory}:
}
}
}
Expand All @@ -167,6 +184,7 @@ type ScanSafeCommitmentsResult struct {
MMRProof merkle.SimplifiedMMRProof
BlockHash types.Hash
Depth uint64
IsMandatory bool
Error error
}

Expand Down Expand Up @@ -231,7 +249,7 @@ func scanSafeCommitments(ctx context.Context, meta *types.Metadata, api *gsrpc.S
select {
case <-ctx.Done():
return
case out <- ScanSafeCommitmentsResult{result.SignedCommitment, proof, blockHash, result.Depth, nil}:
case out <- ScanSafeCommitmentsResult{result.SignedCommitment, proof, blockHash, result.Depth, result.IsMandatory, nil}:
}

}
Expand Down Expand Up @@ -293,3 +311,9 @@ func verifyProof(meta *types.Metadata, api *gsrpc.SubstrateAPI, proof merkle.Sim

return actualRoot == expectedRoot, nil
}

func decodeEngineId(engineId uint32) string {
idAsBytes := make([]byte, 4)
binary.LittleEndian.PutUint32(idAsBytes, engineId)
return string(idAsBytes)
}
3 changes: 2 additions & 1 deletion web/packages/test/config/beefy-relay.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,8 @@
},
"beefy-activation-block": 0,
"fast-forward-depth": 20,
"update-period": 0
"update-period": 0,
"discard-non-mandatory-commitments": false
},
"sink": {
"ethereum": {
Expand Down
1 change: 0 additions & 1 deletion web/packages/test/scripts/start-relayer.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ config_relayer() {
--arg eth_gas_limit $eth_gas_limit \
'
.sink.contracts.BeefyClient = $k1
| .source.ethereum.endpoint = $eth_endpoint_ws
| .sink.ethereum.endpoint = $eth_endpoint_ws
| .sink.ethereum."gas-limit" = $eth_gas_limit
' \
Expand Down