Basic IBC Handshake Test #4797
Basic IBC Handshake Test #4797
Conversation
zbuc
force-pushed
the
ibc_sm_tests
branch
14 times, most recently
from
28f1623
to
8f87b28
Compare
There was a problem hiding this comment.
Excellent work, dude. So glad to see this work coming together, and some query bugs squashed along the way. Please keep the in-line comments coming, because we discovered a lot of subtlety the past few days, and you're in a great position to reduce surprises for future maintainers by documenting your travails.
|
|
||
| /// Collection of all keypairs required for a Penumbra validator. | ||
| /// Used to generate a stable identity for a [`NetworkValidator`]. | ||
| /// TODO: copied this from pd crate |
There was a problem hiding this comment.
N.B. I've long wanted to pull out most of the network code from pd into a new crate penumbra-network, and in the process clean up the interfaces for wrangling the validator keyrings.
| @@ -175,6 +175,48 @@ pub struct ValidatorKeys { | |||
| } | |||
|
|
|||
| impl ValidatorKeys { | |||
| /// Use a hard-coded seed to generate a new set of validator keys. | |||
| pub fn from_seed(seed: [u8; 32]) -> Self { | |||
There was a problem hiding this comment.
Very cool to have this utility!
| @@ -198,7 +240,7 @@ impl ValidatorKeys { | |||
| let validator_cons_pk = validator_cons_sk.public_key(); | |||
|
|
|||
| // generate P2P auth key for tendermint. | |||
| let node_key_sk = ed25519_consensus::SigningKey::new(OsRng); | |||
| let node_key_sk = ed25519_consensus::SigningKey::from(seed.0); | |||
There was a problem hiding this comment.
I assume the hardcoding of a static seed is leftover from debugging—for the actual test suite, we should probably leave the keygen to be random as usual. Fine for now, because I know there'll be a lot of work coming on top of this. Mentioning it now in case you disagree with the end goal.
| tonic::Status::aborted(format!("couldn't decode height: {e}")) | ||
| })?, | ||
| }), | ||
| }; |
There was a problem hiding this comment.
Regarding the get_block_height + 1 logic throughout, that unblocked so much of these state machine tests, we should be documenting those sharp edges more comprehensively. Not blocking merge, but IMO, explaining the difference in height queries in the ibc module docs makes the most sense: it'll be immediately obvious to any developers looking at that code that the various notions of height are important and distinct.
Describe your changes
This adds a basic IBC handshake test, using the existing mock client. Some opportunistic refactoring is also included.
This PR adds a new
tests::common::ibc_testsmodule, which contains aMockRelayerthat can be extended later on.Follow-up tasks should be basic transfer testing, transfer timeout testing, and testing with malformed requests.
While debugging this test, bugs were found in the various IBC query APIs, specifically that the
proof_heightwas consistently being returned one lower than the height whose header would contain the app_hash necessary for validating the proof. The Go relayer is unaffected because it uses the ABCI RPC query interface instead, and Hermes uses the affected APIs but discards the affectedproof_heightfields and uses its own internal mechanisms for height tracking instead. Fixes were included for the affected APIs.Issue
Closes #3758
Checklist before requesting a review
If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason: