feat: add block by number and remove epoch accumulators #1436
Conversation
KolbyML
force-pushed
the
block-number
branch
2 times, most recently
from
d3da8b0 to
bd5713e
Compare
This comment was marked as outdated.
This comment was marked as outdated.
KolbyML
force-pushed
the
block-number
branch
2 times, most recently
from
df74e63 to
ec2cf26
Compare
|
|
||
| /// A content key in the history overlay network. | ||
| #[derive(Clone, Debug, Eq, PartialEq)] | ||
| pub enum HistoryContentKey { | ||
| /// A block header with accumulator proof. | ||
| BlockHeaderWithProof(BlockHeaderKey), | ||
| BlockHeaderByHashWithProof(BlockHeaderByHashKey), |
There was a problem hiding this comment.
I'm in favor of assuming that the WithProof part is implied and changing these to BlockHeaderByHash & BlockHeaderByNumber
| @@ -52,7 +52,7 @@ pub async fn test_invalidate_header_by_hash(peertest: &Peertest, target: &Client | |||
|
|
|||
There was a problem hiding this comment.
I feel like we should add a test_validate_pre_merge_block_header_by_number() scenario here.
| assert_eq!(key.content_id(), expected_content_id); | ||
| assert_eq!( | ||
| key.to_string(), | ||
| "BlockHeaderByNumberWithProof { block_number: 12345678 }" |
There was a problem hiding this comment.
I'd also add the round trip case, that was in the old epoch acc test, to both this test and to block_header_by_hash. Basically testing that the HistoryContentKey::try_from() case works as expected
| ); | ||
| assert_eq!(key.to_hex(), KEY_STR); | ||
| } | ||
|
|
||
| #[test] | ||
| fn ser_de_block_header() { |
There was a problem hiding this comment.
Seems like it's worth duplicating this case to cover the by_number case as well
portal-bridge/src/bridge/era1.rs
Outdated
| @@ -287,27 +291,12 @@ impl Era1Bridge { | |||
| } | |||
|
|
|||
| async fn get_epoch_acc(&self, epoch_index: u64) -> anyhow::Result<Arc<EpochAccumulator>> { | |||
There was a problem hiding this comment.
tbh, now that this fn is so small, you can probably just inline lookup_epoch_acc wherever it's needed
trin-history/src/validation.rs
Outdated
| return Err(anyhow!( | ||
| "Content validation failed: Invalid header number. Found: {header_number} - Expected: {}", | ||
| key.block_number | ||
| )); |
There was a problem hiding this comment.
Seems like there's some alignment issues going on here
| .is_err()); | ||
| assert!(HistoryNetworkApiClient::local_content( | ||
| &peertest.nodes[0].ipc_client, | ||
| receipts_key_1.clone() | ||
| ) | ||
| .await | ||
| .is_err()); | ||
| // check that peertest bootnode does not have accumulator_1 |
| // Store accumulator_2 locally in target | ||
| let (acc_key_2, acc_value_2) = fixture_epoch_acc_2(); | ||
| // Store block_2 minus receipts locally in target | ||
| let (header_key_2, header_value_2) = fixture_header_by_hash_with_proof_15040709(); |
There was a problem hiding this comment.
I don't think you actually need to store the header locally in every target. As long as it's available in any single node, it should be available to the others for validation.
There was a problem hiding this comment.
I am confused, am I not just storing the header in target?
| @@ -90,37 +91,99 @@ pub async fn test_gossip_dropped_with_offer(peertest: &Peertest, target: &Client | |||
| .unwrap(); | |||
| let fresh_enr = fresh_target.node_info().await.unwrap().enr; | |||
|
|
|||
| // Store accumulator_1 locally in client that is not connected to the network | |||
| let (acc_key_1, acc_value_1) = fixture_epoch_acc_1(); | |||
| // Store block_1 locally in client that is not connected to the network | |||
There was a problem hiding this comment.
Sorry, but I realize now that the large content values I gave were not ideal, and it shows in the complexity of this test, making it difficult to follow what's going on...
I updated #1438 with new assets that should simplify this test...
You should be able to write a test that's accurately testing the gossip-dropped feature using only the..
- receipts from block
15040641(320kb) - body from block
15040708(763kb) - (and the 2 headers for validation)
As it stands, there's just too much going on and it's tough to follow the exact code paths that are being tested
|
@njgheorghita ready for another look |
| @@ -202,18 +277,36 @@ pub async fn test_gossip_dropped_with_find_content(peertest: &Peertest, target: | |||
| .await | |||
| .unwrap(); | |||
|
|
|||
| // Store accumulator_1 locally in client that is not connected to the network | |||
| let (acc_key_1, acc_value_1) = fixture_epoch_acc_1(); | |||
| // Store receipts_1 locally in client that is not connected to the network | |||
There was a problem hiding this comment.
I would update this comment
Store receipts_1 locally in client, without validation, that is not connected to the network
And then there's no need to store header_key_1 in fresh_target
| .unwrap(); | ||
| assert!(store_result); | ||
|
|
||
| // Store body_2 locally in target |
There was a problem hiding this comment.
Here I would also store header_key_1 in target, so that it's available for validation
| .await | ||
| .unwrap(); | ||
|
|
||
| // check that the fresh target has stored accumulator_2 | ||
| // check that the fresh target has stored body_2 stored |
There was a problem hiding this comment.
And then here, I don't really think it's necessary to check that any nodes have any header values....
The only values that we're really concerned about here are the large receipts/bodies.
We want to isolate the feature being tested on fresh_target as much as possible, so I think it's better to remove a lot of the checks for headers, bc it's a bit lengthy.
The only situation where I think these header checks would be useful is if the header is not available, anywhere on the peertest, for validation, which would break gossip. But, with the large, unrestricted nodes, this should never happen, since they'll never prune the headers.
| @@ -75,7 +75,12 @@ impl ExecutionApi { | |||
| &self, | |||
| height: u64, | |||
| epoch_acc: Option<Arc<EpochAccumulator>>, | |||
| ) -> anyhow::Result<(FullHeader, HistoryContentKey, HistoryContentValue)> { | |||
| ) -> anyhow::Result<( | |||
There was a problem hiding this comment.
The docstring for this function needs updating
| block_hash: header_content_value.header.hash().0, | ||
| }); | ||
| let content_value = | ||
| HistoryContentValue::decode(&content_key, &dependent_value).unwrap(); |
There was a problem hiding this comment.
shouldn't clippy be complaining about this unwrap?
There was a problem hiding this comment.
This is in ethportal-peertest, I thought testing stuff didn't have that check
portal-bridge/src/bridge/history.rs
Outdated
| @@ -341,25 +355,12 @@ impl HistoryBridge { | |||
| &self, | |||
There was a problem hiding this comment.
I'd also get rid of this function and just inline the relevant code since there's no more gossiping going on
portal-bridge/src/bridge/era1.rs
Outdated
| @@ -246,13 +250,20 @@ impl Era1Bridge { | |||
| panic!("Failed to get epoch from era1 file: {e}"); | |||
| } | |||
| }; | |||
| let epoch_acc = match self.get_epoch_acc(epoch_index).await { | |||
| let (_, epoch_acc) = match lookup_epoch_acc( | |||
There was a problem hiding this comment.
It doesn't seem like we're using the epoch_hash that's being returned as the first value anymore, so you could just update this util fn to return the epoch_acc
trin-history/src/validation.rs
Outdated
| HistoryContentKey::BlockHeaderByNumber(key) => { | ||
| let header_with_proof = | ||
| HeaderWithProof::from_ssz_bytes(content).map_err(|err| { | ||
| anyhow!("Header with proof content has invalid encoding: {err:?}") |
There was a problem hiding this comment.
Maybe it'd be useful to differentiate this error message from the one on line 35
| #[case(1_000_009)] | ||
| #[case(1_000_010)] | ||
| #[tokio::test] | ||
| async fn generate_and_verify_header_with_proofs(#[case] block_number: u64) { |
There was a problem hiding this comment.
This test still seems somewhat useful to leave in. We still want to test that trin constructs & validates (and invalidates) these header with proofs...
I would just update this test (& invalidate_invalid_proofs) to lookup the epoch accs directly from disk and use PreMergeAccumulator::construct_proof() instead of the removed PreMergeAccumulator::generate_proof()
trin-history/src/validation.rs
Outdated
| @@ -162,7 +151,7 @@ mod tests { | |||
| let hwp = HeaderWithProof::from_ssz_bytes(&hwp_ssz).expect("error decoding header"); | |||
There was a problem hiding this comment.
Seems worthwhile at least adding a simple validate/invalidate test for BlockHeaderWithNumber here
KolbyML
force-pushed
the
block-number
branch
from
95e63b3 to
c5af146
Compare
|
@njgheorghita ready for another look |
There was a problem hiding this comment.
Looks good! One last thing to consider before merging... we still have some number of epoch accumulators out there in the dbs of our network nodes. I think we want to drop all of those out before deploying this change. So I think we should hold off on merging this until we've done the filtering. I can dust off #1286 tomorrow and then once that's deployed / the filtering is done, we can merge this pr
| let expected_content_key = HistoryContentKey::BlockHeaderWithProof(BlockHeaderKey { | ||
| block_hash: BLOCK_HASH, | ||
| fn ser_de_block_header_by_number() { | ||
| // let content_key_json = "12345678"; |
There was a problem hiding this comment.
Should we update the comment here to be more clear?
|
|
||
| /// A content key in the history overlay network. | ||
| #[derive(Clone, Debug, Eq, PartialEq)] | ||
| pub enum HistoryContentKey { | ||
| /// A block header with accumulator proof. | ||
| BlockHeaderWithProof(BlockHeaderKey), | ||
| BlockHeaderByHash(BlockHeaderByHashKey), | ||
| /// A block header with accumulator proof. |
There was a problem hiding this comment.
This docstring should be updated.
There was a problem hiding this comment.
Moving ahead with the plan from #1286 (comment), so this is good to go on my end after @ogenev comments are addressed
What was wrong?
implements ethereum/portal-network-specs#334
How was it fixed?
by removing our uses of epoch accumulators as a content type
I reworked our peertests which used epochaccumulators using some test data Nick gave me to use other history types instead