feat: implement misbehaviour program #103

gjermundgaraba · 2024-08-28T13:55:57Z

Closes #56

There are a couple of only semi-related changes that I will comment on in the changes.

gjermundgaraba · 2024-08-28T13:56:58Z

.github/workflows/e2e.yml

        with:
-          version: v1.59
+          version: v1.60


I changed to use 1.60 because there is a memory bug in 1.59 that causes a lot of problems on my machine.

The version bumps in this file is just because I thought it was good to have latest version 🤷

gjermundgaraba · 2024-08-28T13:58:28Z

contracts/script/SP1ICS07Tendermint.s.sol

@@ -61,13 +63,15 @@ contract SP1TendermintScript is Script, IICS07TendermintMsgs {
        bytes32 updateClientVkey = json.readBytes32(".updateClientVkey");
        bytes32 membershipVkey = json.readBytes32(".membershipVkey");
        bytes32 ucAndMembershipVkey = json.readBytes32(".ucAndMembershipVkey");
+        bytes32 misbehaviourVkey = json.readBytes32(".misbehaviourVkey");


Since this is a new program, it means we need a new program key to verify it in the proof verification process in Solidity.

gjermundgaraba · 2024-08-28T13:59:54Z

contracts/src/SP1ICS07Tendermint.sol

+        }
+        validateEnv(output.env);
+
+        bytes32 outputConsensusStateHash1 = keccak256(abi.encode(output.trustedConsensusState1));


Here we basically verify that the trusted consensus state in both headers are known (trusted) by us already.

In code docs will make it clearer for new readers why this is needed

gjermundgaraba · 2024-08-28T14:02:23Z

e2e/interchaintestv8/.golangci.yml

@@ -51,6 +51,10 @@ issues:
  max-same-issues: 10000

 linters-settings:
+  gosec:


The new version of golangci-lint has a really aggressive check for casting of ints, which we have to do a lot with heights (and there is no "safe" way that will shut it up)

(For context, I think this check has removed from the default set in for the next release of golangci-lint, so others are also complaining about it)

gjermundgaraba · 2024-08-28T14:05:48Z

e2e/interchaintestv8/sp1_ics07_test.go

@@ -34,6 +38,9 @@ import (
 type SP1ICS07TendermintTestSuite struct {
 	e2esuite.TestSuite

+	// Whether to generate fixtures for the solidity tests
+	generateFixtures bool


I added this to be able to generate fixtures from the e2e tests for misbehaviour. We had already talked about potentially doing this for all fixtures, so we don't depend on getting state from the mocha testnet as we do now. Instead we could generate them from the e2e tests which should be perfectly valid. We already do this for benchmarks in the solidity-ibc-eureka repo.

gjermundgaraba · 2024-08-28T14:07:01Z

justfile

+    "RUST_LOG=info SP1_PROVER={{prover}} ./target/release/operator fixtures update-client --trusted-block $TRUSTED_HEIGHT --target-block $TARGET_HEIGHT -o 'contracts/fixtures/update_client_fixture.json'" \
+    "sleep 15 && RUST_LOG=info SP1_PROVER={{prover}} ./target/release/operator fixtures update-client-and-membership --key-paths clients/07-tendermint-0/clientState,clients/07-tendermint-001/clientState --trusted-block $TRUSTED_HEIGHT --target-block $TARGET_HEIGHT -o 'contracts/fixtures/uc_and_memberships_fixture.json'" \
+    "sleep 30 && RUST_LOG=info SP1_PROVER={{prover}} ./target/release/operator fixtures membership --key-paths clients/07-tendermint-0/clientState,clients/07-tendermint-001/clientState --trusted-block $TRUSTED_HEIGHT -o 'contracts/fixtures/memberships_fixture.json'"
+  cd e2e/interchaintestv8 && RUST_LOG=info SP1_PROVER=network GENERATE_FIXTURES=true go test -v -run '^TestWithSP1ICS07TendermintTestSuite/TestMisbehaviour$' -timeout 40m


I ran into some issues when trying to run this one in parallel, but it is quite fast, so not as big of a deal.

AdityaSripal

Great work!! It took me a while to understand what was happening and to dive into the ibc-rs code as well to see how it all fit together.

Think some code docs will help a lot and i left a few suggestions. Some may be obvious to rust readers which can be omitted in that case.

I will review the tests in a second pass but the implementation code lgtm!

programs/misbehaviour/src/lib.rs

AdityaSripal · 2024-08-29T16:23:14Z

programs/misbehaviour/src/lib.rs

+
+    assert!(is_misbehaviour, "Misbehaviour is not detected");
+
+    let output_trusted_header_1 = sp1_ics07_tendermint::Height {


Can we name this variable output_trusted_height_1

That would make more sense 🙈

AdityaSripal · 2024-08-29T16:23:18Z

programs/misbehaviour/src/lib.rs

+            .try_into()
+            .unwrap(),
+    };
+    let output_trusted_header_2 = sp1_ics07_tendermint::Height {


AdityaSripal · 2024-08-29T16:36:37Z

operator/src/runners/fixtures/misbehaviour.rs

+        args.trust_options.trust_level,
+    )
+    .await?;
+    let genesis_2 = SP1ICS07TendermintGenesis::from_env(


Use trusted light block 2 to instantiate a new SP1 tendermint client with light block 2 as initial trusted consensus state

AdityaSripal · 2024-08-29T16:37:08Z

operator/src/runners/fixtures/misbehaviour.rs

+    )
+    .await?;
+
+    let trusted_consensus_state_1 =


Use the clients to convert the Tendermint light blocks into the IBC Tendermint trusted consensus states

AdityaSripal · 2024-08-29T16:37:26Z

operator/src/runners/fixtures/misbehaviour.rs

+        ConsensusState::abi_decode(&genesis_1.trusted_consensus_state, false)?;
+    let trusted_consensus_state_2 =
+        ConsensusState::abi_decode(&genesis_2.trusted_consensus_state, false)?;
+    let trusted_client_state_2 = ClientState::abi_decode(&genesis_2.trusted_client_state, false)?;


Use the client state from genesis_2 as the client state since they will both be the same

AdityaSripal · 2024-08-29T16:37:55Z

operator/src/runners/fixtures/misbehaviour.rs

+
+    let verify_misbehaviour_prover = SP1ICS07TendermintProver::<MisbehaviourProgram>::default();
+
+    let contract_env = Env {


construct contract env from the client state

AdityaSripal · 2024-08-29T16:50:51Z

contracts/src/SP1ICS07Tendermint.sol

+        }
+        validateEnv(output.env);
+
+        bytes32 outputConsensusStateHash1 = keccak256(abi.encode(output.trustedConsensusState1));


In code docs will make it clearer for new readers why this is needed

Co-authored-by: Aditya <[email protected]>

gjermundgaraba · 2024-08-30T10:47:46Z

Great work!! It took me a while to understand what was happening and to dive into the ibc-rs code as well to see how it all fit together.

Think some code docs will help a lot and i left a few suggestions. Some may be obvious to rust readers which can be omitted in that case.

I will review the tests in a second pass but the implementation code lgtm!

Thank you! Really good suggestions! I have added all the requested comment points. Let me know if you would like any other places documented better or any other changes you would like to see :)

AdityaSripal

Super super impressed with the testing setup you guys have built!!! I have a couple nits and one more test case to check but everything else looks great

AdityaSripal · 2024-09-02T18:12:12Z

e2e/interchaintestv8/e2esuite/utils.go

+// It uses the given oldHeader as a base to create the new header with a new hash, and sign using the existing validators
+// It can be used to for instance test misbehaviour
+// Partially based on https://github.com/cosmos/relayer/blob/f9aaf3dd0ebfe99fbe98d190a145861d7df93804/interchaintest/misbehaviour_test.go#L38
+func (s *TestSuite) CreateTMClientHeader(


impressive work!

AdityaSripal · 2024-09-02T18:19:07Z

e2e/interchaintestv8/operator/operator.go

+
+	validators1 := jsonIntermediary["header_1"].(map[string]interface{})["validator_set"].(map[string]interface{})["validators"].([]interface{})
+	validators2 := jsonIntermediary["header_2"].(map[string]interface{})["validator_set"].(map[string]interface{})["validators"].([]interface{})
+	validators3 := jsonIntermediary["header_1"].(map[string]interface{})["trusted_validators"].(map[string]interface{})["validators"].([]interface{})


these names should trustedValidators1 or similar

AdityaSripal · 2024-09-02T18:35:31Z

e2e/interchaintestv8/sp1_ics07_test.go

+		s.Require().ErrorContains(err, "Misbehaviour is not detected")
+	}))
+
+	s.Require().True(s.Run("Valid misbehaviour", func() {


Can we add a test with time montonicity breaking. ie, next header has a lower time

Yes, that makes perfect sense!

operator/src/runners/fixtures/misbehaviour.rs

Co-authored-by: Aditya <[email protected]>

crodriguezvega

There's a lot to unpack, but this is fantastic work. Well done, @gjermundgaraba!

contracts/src/SP1ICS07Tendermint.sol

crodriguezvega · 2024-09-03T07:00:49Z

contracts/fixtures/e2e_misbehaviour_fixture.json

@@ -0,0 +1,9 @@
+{


Maybe rename file to misbehaviour_fixture.json? (to keep naming consistent with the other files in the folder - alternatively rename the other files to prepend e2e 😄)

Yeah, it makes sense. It was just to signify a difference as the other ones are currently generated from Celestia mocha testnet. But I think it still makes sense to rename this one to misbehaviour_fixture.json to keep naming consistent 👍

crodriguezvega · 2024-09-03T07:14:00Z

contracts/test/Misbehaviour.t.sol

+
+    function test_ValidMisbehaviour() public {
+        // set a correct timestamp
+        vm.warp(env.now + 300);


Why adding 300 sets the correct timestamp?

300 is slightly arbitrary, but the idea was that slightly in the future is more realistic as the env.now is the timestamp when the proof was generated, and the misbehaviour will be submitted later.

contracts/test/Misbehaviour.t.sol

crodriguezvega · 2024-09-03T07:26:11Z

e2e/interchaintestv8/e2esuite/utils.go

@@ -133,6 +145,132 @@ func (s *TestSuite) GetTxReciept(ctx context.Context, chain *ethereum.EthereumCh

 		return receipt != nil, nil
 	})
+
+	// TODO: This should check if the tx was actually successful and return a bool or err on that basis


To address this TODO, do we basically need to check if receipt.Status == ethtypes.ReceiptStatusSuccessful? (can be done in a separate PR).

Pretty much!

e2e/interchaintestv8/sp1_ics07_test.go

crodriguezvega · 2024-09-03T07:57:13Z

e2e/interchaintestv8/sp1_ics07_test.go

+	_ = eth
+
+	var height clienttypes.Height
+	var oldHeader tmclient.Header


Suggested change

var oldHeader tmclient.Header

var trustedHeader tmclient.Header

Indeed! Fixing this

crodriguezvega · 2024-09-03T07:57:44Z

e2e/interchaintestv8/sp1_ics07_test.go

+
+	var height clienttypes.Height
+	var oldHeader tmclient.Header
+	s.Require().True(s.Run("Get old header", func() {


We are basically getting a trusted header, right?

Suggested change

s.Require().True(s.Run("Get old header", func() {

s.Require().True(s.Run("Get trusted header", func() {

crodriguezvega · 2024-09-03T08:13:54Z

justfile

@@ -29,40 +31,40 @@ install-operator:
  @echo "Installed the operator executable"

 # Run the Solidity tests using `forge test` command
-test-foundry:
-  forge test -vvv
+test-foundry testname=".\\*":


Should we still have a run all tests command anyway?

If no name is passed in it will use the default value which runs all tests

crodriguezvega · 2024-09-03T08:42:40Z

programs/misbehaviour/src/lib.rs

+        trust_threshold: env.trustThreshold.clone().into(),
+        trusting_period: Duration::from_secs(env.trustingPeriod.into()),
+        clock_drift: Duration::default(),


In production environments, these values should come form the client state, right?

Yes, but in the proof program all values need to be passed in. So the operator/relayer is responsible for setting these values. The operator currently gets this by querying a light block of the trusted height.

Co-authored-by: Carlos Rodriguez <[email protected]>

AdityaSripal

Your new test looks good. Think you may have misunderstood my nit. Fix or leave as you see fit

AdityaSripal · 2024-09-03T16:08:12Z

e2e/interchaintestv8/operator/operator.go

+	trustedValidators1 := jsonIntermediary["header_1"].(map[string]interface{})["validator_set"].(map[string]interface{})["validators"].([]interface{})
+	trustedValidators2 := jsonIntermediary["header_2"].(map[string]interface{})["validator_set"].(map[string]interface{})["validators"].([]interface{})
+	trustedValidators3 := jsonIntermediary["header_1"].(map[string]interface{})["trusted_validators"].(map[string]interface{})["validators"].([]interface{})
+	trustedValidators4 := jsonIntermediary["header_2"].(map[string]interface{})["trusted_validators"].(map[string]interface{})["validators"].([]interface{})


Suggested change

trustedValidators1 := jsonIntermediary["header_1"].(map[string]interface{})["validator_set"].(map[string]interface{})["validators"].([]interface{})

trustedValidators2 := jsonIntermediary["header_2"].(map[string]interface{})["validator_set"].(map[string]interface{})["validators"].([]interface{})

trustedValidators3 := jsonIntermediary["header_1"].(map[string]interface{})["trusted_validators"].(map[string]interface{})["validators"].([]interface{})

trustedValidators4 := jsonIntermediary["header_2"].(map[string]interface{})["trusted_validators"].(map[string]interface{})["validators"].([]interface{})

validators1 := jsonIntermediary["header_1"].(map[string]interface{})["validator_set"].(map[string]interface{})["validators"].([]interface{})

validators2 := jsonIntermediary["header_2"].(map[string]interface{})["validator_set"].(map[string]interface{})["validators"].([]interface{})

trustedValidators1 := jsonIntermediary["header_1"].(map[string]interface{})["trusted_validators"].(map[string]interface{})["validators"].([]interface{})

trustedValidators2 := jsonIntermediary["header_2"].(map[string]interface{})["trusted_validators"].(map[string]interface{})["validators"].([]interface{})

Ah! Yeah of course, that makes sense 🙈 Fixing it!

gjermundgaraba added 8 commits July 28, 2024 12:39

initial implementation of the misbehaviour program

16df45a

Merge branch 'main' into gjermund/56-implement-misbehavior-program

0c329f8

misbehaviour program with operator integration with fixture output

269bb6a

lint

4ac34e9

output misbehaviour fixture from e2e test

849d020

misbehaviour tests (unit and e2e)

dcb3e73

some code cleanup

91aee68

code cleanup

bf39613

gjermundgaraba requested a review from srdtrk as a code owner August 28, 2024 13:55

gjermundgaraba linked an issue Aug 28, 2024 that may be closed by this pull request

Implement misbehavior program #56

Closed

gjermundgaraba requested review from crodriguezvega and AdityaSripal August 28, 2024 13:56

gjermundgaraba commented Aug 28, 2024

View reviewed changes

gjermundgaraba added 4 commits August 28, 2024 16:29

added clippy linting

5f750fa

Added misbehaviour program to ci build pipeline

c9cbfb3

fix cargo setups for misbehaviour

4f3864e

lint and add e2e test to ci

46dfe66

AdityaSripal reviewed Aug 29, 2024

View reviewed changes

gjermundgaraba and others added 5 commits August 30, 2024 12:27

Update programs/misbehaviour/src/lib.rs

588f580

Co-authored-by: Aditya <[email protected]>

Update programs/misbehaviour/src/lib.rs

df06cd8

Co-authored-by: Aditya <[email protected]>

Update programs/misbehaviour/src/lib.rs

f36b596

Co-authored-by: Aditya <[email protected]>

Update programs/misbehaviour/src/lib.rs

f0d9b70

Co-authored-by: Aditya <[email protected]>

pr review fixes

aff47cc

AdityaSripal approved these changes Sep 2, 2024

View reviewed changes

Update operator/src/runners/fixtures/misbehaviour.rs

b7b8d0e

Co-authored-by: Aditya <[email protected]>

crodriguezvega approved these changes Sep 3, 2024

View reviewed changes

gjermundgaraba and others added 4 commits September 3, 2024 12:36

Update e2e/interchaintestv8/sp1_ics07_test.go

5676c8e

Co-authored-by: Carlos Rodriguez <[email protected]>

code review fixes

8947815

added test for misbehaviour breaking time monotonicity

4b8e72b

added unit test for TrustingPeriodTooLong

e6ae78f

gjermundgaraba requested review from crodriguezvega and AdityaSripal September 3, 2024 15:59

AdityaSripal approved these changes Sep 3, 2024

View reviewed changes

rename validators correctly

0e641ce

gjermundgaraba merged commit 8102e5f into main Sep 4, 2024
16 checks passed

gjermundgaraba deleted the gjermund/56-implement-misbehavior-program branch September 4, 2024 14:20


		assert!(is_misbehaviour, "Misbehaviour is not detected");

		let output_trusted_header_1 = sp1_ics07_tendermint::Height {


		let verify_misbehaviour_prover = SP1ICS07TendermintProver::<MisbehaviourProgram>::default();

		let contract_env = Env {

	var oldHeader tmclient.Header
	var trustedHeader tmclient.Header

	s.Require().True(s.Run("Get old header", func() {
	s.Require().True(s.Run("Get trusted header", func() {

feat: implement misbehaviour program #103

feat: implement misbehaviour program #103

Conversation

gjermundgaraba commented Aug 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gjermundgaraba Aug 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AdityaSripal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gjermundgaraba commented Aug 30, 2024

AdityaSripal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AdityaSripal Sep 2, 2024 • edited Loading

Choose a reason for hiding this comment

gjermundgaraba Sep 3, 2024 • edited Loading

Choose a reason for hiding this comment

crodriguezvega left a comment

Choose a reason for hiding this comment

crodriguezvega Sep 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AdityaSripal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gjermundgaraba Aug 28, 2024 •

edited

Loading

AdityaSripal Sep 2, 2024 •

edited

Loading

gjermundgaraba Sep 3, 2024 •

edited

Loading

crodriguezvega Sep 3, 2024 •

edited

Loading