NetUpgrade in the ChainConfig. - #65
Conversation
Fix couple of old fields and add nonce field to Ping/Pong so that the protocol is slightly more robust when accepting a Pong from remote peer.
Ping/Pong is used to test the aliveness of the remote peer and it works by periodically sending a Ping message and expecting a response to it within some predefined time period. This commit implements this functionality as a state machine on top of the asynchronous protocol by introducing substates to the default listening state of the `PeerState` enum. This allows the Ping/Pong functionality to follow a certain path and catch errors where appropriate while still allowing the remote peer to essentially send anything they want, as long as they reply to the Ping message in time and with correct nonce.
Instead of matching both state and message type at the same time, match the state first, then call an appropriate function for that state and match the message type in that function to handle the (state, message type) combination. Additionally, add some comments, reword an error message and rename a few tests functions to be more descriptive.
rename util directory to uint; move the uint file to the uint directory; ran clippy. for compact to uint256, `TryFrom` is implemented
…segregated the tests; added max function on BlockHeight and implement saturating for the same type
|
@TheQuantumPhysicist is it correct to assume: |
| @@ -27,6 +28,8 @@ pub struct ChainConfig { | |||
| #[allow(dead_code)] | |||
| height_checkpoint_data: BTreeMap<BlockHeight, HashType>, | |||
There was a problem hiding this comment.
Please change HashType from Vec<u8> to H256.
| } | ||
|
|
||
| //Note: it doesn't have to be 1000. Could be 1000, or 10000 | ||
| min_height = min_height.saturating_sub(1000); |
There was a problem hiding this comment.
This is very wrong... you should never use O(N) complexity like this. Worst case scenario, you use the BTreeMap and then do a lower_bound algorithm while handling corner cases.
This is an article about it on stackoverflow:
https://stackoverflow.com/questions/48575866/how-to-get-the-lower-bound-and-upper-bound-of-an-element-in-a-btreeset
This comes out of the box in C++... I don't know if it's there in rust:
https://www.cplusplus.com/reference/map/map/lower_bound/
|
|
||
| fn version_num_from_height(&self, height: BlockHeight) -> NetUpgradeVersion { | ||
| { | ||
| if height == BlockHeight::zero() { |
There was a problem hiding this comment.
Why doesn't version_type_from_height handle zero?!
height.rs : fix clippy issue config.rs : apply comments from Sam about HashType to H256; and adding block zero by default in the net_upgrades field of ChainConfig
| } | ||
| } | ||
|
|
||
| pub trait NetUpgradesExt { |
There was a problem hiding this comment.
Why do we need this trait? Why not just impl NetUprageConfig.
There was a problem hiding this comment.
well in the ChainConfig, the netupgrade field looks like this: BTreeMap<BlockHeight, NetUpgradeType>
The NetUpgradeConfig is generated from the NetUpgradeType and theChainType.
The functions is_upgrade_activated and height_range version_type_from_height are for the Btreemap, not the config.
I should probably rename NetUpgradeConfig to.. ExtraConfig or something, which is still an enum.
There was a problem hiding this comment.
basically I've treated POW and POS as the same version (in terms of number. Like, these types are both version 1) but their configurations are different. The only time POW or POS configuration is shown, is from a "debug" of ChainConfig {:?}, or probably when saving this as a json and into a file or something.
I'll check on that tomorrow, first thing, the debug.
|
I think there was a problem with rebase because it seems you've co-authored some P2P commits and in doing so, the commit hashes have changed so git treats them as different commits. I don't now how to fix this in Github but you can create a new branch that has the latest master, cherry-pick your changes from this branch to that branch, close this PR and create a new PR for the new branch. |
implement `TryFrom` for POWConfig, to generate this config from the upgrade version and the chain_type. simplify netupgrade. Moved to its own package in the chain package.
This is continuation of #65 implement `TryFrom` for POWConfig, to generate this config from the upgrade version and the chain_type. simplify netupgrade. Moved to its own package in the chain package.
3 participants
netupgrades, initial push.
TODO: provide the height boundaries of an upgrade.This draft PR needs #64