add chain id & chain config check to peering process #785
Conversation
leviathanbeak
deleted the
leviathanbeak/add_chain_id_to_peering_process
branch
|
Ah, wait, why so fast, I almost finished my comment |
| @@ -66,13 +74,19 @@ const MAX_NUM_OF_FRAMES_BUFFERED: usize = 256; | |||
| /// inbound and outbound connections established through the transport. | |||
| const TRANSPORT_TIMEOUT: Duration = Duration::from_secs(20); | |||
|
|
|||
| /// Sha256 hash of chain id and chain config | |||
| type Checksum = [u8; 32]; | |||
There was a problem hiding this comment.
It is better to use its own type to ensure that the checksum was correctly created.
| fn from(args: P2pArgs) -> Self { | ||
| impl P2pArgs { | ||
| pub fn into_config(self, chain_config: &ChainConfig) -> anyhow::Result<P2PConfig> { | ||
| let checksum = *fuel_crypto::Hasher::default() |
There was a problem hiding this comment.
I think we need to use the same serialization/merklezation mechanism as we use for the genesis block here.
There was a problem hiding this comment.
Hmm this fn you linked to doesn't include the initial state root or chain name though
There was a problem hiding this comment.
Then, maybe better to use the root of the genesis block=)
Regarding the name of the network, we can include it, if you think that we need this kind of information in the ChainConfig
There was a problem hiding this comment.
Yeah it should be a checksum of the full chainspec / entire genesis block.
Also, these root functions should exist in the chain-config module so that they can be used without fuel-core. As fuel-p2p is in it's own crate and won't be able to use the genesis module directly.
|
@xgreenx sorry, it's late for me, I saw one approval and thought let me close the day with a merge, will address your comments tomorrow, thanks for the review :) |
|
Create an issue not to forget #824 |
closes #646
libp2pallows us to implement Inbound and Outbound upgrades during connection initialization,so I've implemented those 2 traits for our
FuelUpgradein which we compare the checksum (hash of theChainConfig) and if they are not correct the connection is rejected.Wrote a test to confirm that the connection will not be established, all other tests now implicitly confirm that the connection is established if the checksum matches.
Since we needed to pass
ChainConfigvariable intoP2pArgsforP2pConfiginitialization, I had to refactorP2pArgsa bit, now instead of implementing From Trait forP2pArgs -> P2pConfigI created an instance method that consumesP2pArgswith a reference toChainConfigOld:
New: