Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: update portal_*Offer to handle multiple pieces of content #1507

Merged
merged 4 commits into from
Oct 8, 2024
Merged

feat: update portal_*Offer to handle multiple pieces of content #1507

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ad27680
feat: update `portal_*Offer` to handle multiple pieces of content
KolbyML Oct 2, 2024
6c60229
fix: remove wire_offer and use offer
KolbyML Oct 6, 2024
805f946
fix: use const for max amount of offered content keys
KolbyML Oct 6, 2024
5b50eaf
fix: pr concerns
KolbyML Oct 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 4 additions & 16 deletions ethportal-api/src/beacon.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,19 +115,18 @@ pub trait BeaconNetworkApi {
content_value: RawContentValue,
) -> RpcResult<TraceGossipInfo>;

/// Send an OFFER request with given ContentKey, to the designated peer and wait for a response.
/// Does not store the content locally.
/// Send an OFFER request with given ContentItems, to the designated peer and wait for a
/// response. Does not store the content locally.
/// Returns the content keys bitlist upon successful content transmission or empty bitlist
/// receive.
#[method(name = "beaconOffer")]
async fn offer(
&self,
enr: Enr,
content_key: BeaconContentKey,
content_value: RawContentValue,
content_items: Vec<(BeaconContentKey, RawContentValue)>,
) -> RpcResult<AcceptInfo>;

/// Send an OFFER request with given ContentKey, to the designated peer.
/// Send an OFFER request with given ContentItems, to the designated peer.
/// Does not store the content locally.
/// Returns trace info for the offer.
#[method(name = "beaconTraceOffer")]
Expand All @@ -138,17 +137,6 @@ pub trait BeaconNetworkApi {
content_value: RawContentValue,
) -> RpcResult<OfferTrace>;

/// Send an OFFER request with given ContentKeys, to the designated peer and wait for a
/// response. Requires the content keys to be stored locally.
/// Returns the content keys bitlist upon successful content transmission or empty bitlist
/// receive.
#[method(name = "beaconWireOffer")]
async fn wire_offer(
&self,
enr: Enr,
content_keys: Vec<BeaconContentKey>,
) -> RpcResult<AcceptInfo>;

/// Store content key with a content data to the local database.
#[method(name = "beaconStore")]
async fn store(
Expand Down
20 changes: 4 additions & 16 deletions ethportal-api/src/history.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,19 +101,18 @@ pub trait HistoryNetworkApi {
content_value: RawContentValue,
) -> RpcResult<TraceGossipInfo>;

/// Send an OFFER request with given ContentKey, to the designated peer and wait for a response.
/// Does not store the content locally.
/// Send an OFFER request with given ContentItems, to the designated peer and wait for a
/// response. Does not store the content locally.
/// Returns the content keys bitlist upon successful content transmission or empty bitlist
/// receive.
#[method(name = "historyOffer")]
async fn offer(
&self,
enr: Enr,
content_key: HistoryContentKey,
content_value: RawContentValue,
content_items: Vec<(HistoryContentKey, RawContentValue)>,
) -> RpcResult<AcceptInfo>;

/// Send an OFFER request with given ContentKey, to the designated peer.
/// Send an OFFER request with given ContentItems, to the designated peer.
/// Does not store the content locally.
/// Returns trace info for the offer.
#[method(name = "historyTraceOffer")]
Expand All @@ -124,17 +123,6 @@ pub trait HistoryNetworkApi {
content_value: RawContentValue,
) -> RpcResult<OfferTrace>;

/// Send an OFFER request with given ContentKeys, to the designated peer and wait for a
/// response. Requires the content keys to be stored locally.
/// Returns the content keys bitlist upon successful content transmission or empty bitlist
/// receive.
#[method(name = "historyWireOffer")]
async fn wire_offer(
&self,
enr: Enr,
content_keys: Vec<HistoryContentKey>,
) -> RpcResult<AcceptInfo>;

/// Store content key with a content data to the local database.
#[method(name = "historyStore")]
async fn store(
Expand Down
9 changes: 4 additions & 5 deletions ethportal-api/src/state.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,19 +94,18 @@ pub trait StateNetworkApi {
content_value: RawContentValue,
) -> RpcResult<TraceGossipInfo>;

/// Send an OFFER request with given ContentKey, to the designated peer and wait for a response.
/// Does not store the content locally.
/// Send an OFFER request with given ContentItems, to the designated peer and wait for a
/// response. Does not store the content locally.
/// Returns the content keys bitlist upon successful content transmission or empty bitlist
/// receive.
#[method(name = "stateOffer")]
async fn offer(
&self,
enr: Enr,
content_key: StateContentKey,
content_value: RawContentValue,
content_items: Vec<(StateContentKey, RawContentValue)>,
) -> RpcResult<AcceptInfo>;

/// Send an OFFER request with given ContentKey, to the designated peer.
/// Send an OFFER request with given ContentItems, to the designated peer.
/// Does not store the content locally.
/// Returns trace info for offer.
#[method(name = "stateTraceOffer")]
Expand Down
18 changes: 6 additions & 12 deletions ethportal-api/src/types/jsonrpc/endpoints.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,10 +42,8 @@ pub enum StateEndpoint {
TraceRecursiveFindContent(StateContentKey),
/// params: [content_key, content_value]
Store(StateContentKey, StateContentValue),
/// WireOffer is not supported in the state network, since locally
/// stored values do not contain the proofs necessary for valid gossip.
/// params: [enr, content_key, content_value]
Offer(Enr, StateContentKey, StateContentValue),
/// params: [enr, Vec<(content_key, content_value>)]
Offer(Enr, Vec<(StateContentKey, StateContentValue)>),
/// params: [enr, content_key, content_value]
TraceOffer(Enr, StateContentKey, StateContentValue),
/// params: [enr, content_key, content_value]
Expand Down Expand Up @@ -79,12 +77,10 @@ pub enum HistoryEndpoint {
Gossip(HistoryContentKey, HistoryContentValue),
/// params: [content_key, content_value]
TraceGossip(HistoryContentKey, HistoryContentValue),
/// params: [enr, content_key, content_value]
Offer(Enr, HistoryContentKey, HistoryContentValue),
/// params: [enr, Vec<(content_key, content_value)>]
Offer(Enr, Vec<(HistoryContentKey, HistoryContentValue)>),
/// params: [enr, content_key, content_value]
TraceOffer(Enr, HistoryContentKey, HistoryContentValue),
/// params: [enr, [content_key]]
WireOffer(Enr, Vec<HistoryContentKey>),
/// params: [enr]
Ping(Enr),
/// params: content_key
Expand Down Expand Up @@ -133,12 +129,10 @@ pub enum BeaconEndpoint {
Gossip(BeaconContentKey, BeaconContentValue),
/// params: [content_key, content_value]
TraceGossip(BeaconContentKey, BeaconContentValue),
/// params: [enr, content_key, content_value]
Offer(Enr, BeaconContentKey, BeaconContentValue),
/// params: [enr, Vec<(content_key, content_value>)]
Offer(Enr, Vec<(BeaconContentKey, BeaconContentValue)>),
/// params: [enr, content_key, content_value]
TraceOffer(Enr, BeaconContentKey, BeaconContentValue),
/// params: [enr, [content_key]]
WireOffer(Enr, Vec<BeaconContentKey>),
/// params: enr
Ping(Enr),
/// params: content_key
Expand Down
2 changes: 2 additions & 0 deletions ethportal-api/src/types/portal.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@ pub struct PongInfo {

pub type FindNodesInfo = Vec<Enr>;

pub const MAX_AMOUNT_OF_OFFERED_CONTENT_KEYS: usize = 64;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit MAX_CONTENT_KEYS_PER_OFFER


/// Response for Offer endpoint
#[derive(Clone, Debug, PartialEq, Serialize, Deserialize)]
#[serde(rename_all = "camelCase")]
Expand Down
8 changes: 5 additions & 3 deletions ethportal-peertest/src/scenarios/gossip.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,13 +181,15 @@ pub async fn test_gossip_dropped_with_offer(peertest: &Peertest, target: &Client
target
.offer(
fresh_enr.clone(),
header_key_2.clone(),
header_value_2.encode(),
vec![(header_key_2.clone(), header_value_2.encode())],
)
.await
.unwrap();
target
.offer(fresh_enr.clone(), body_key_2.clone(), body_value_2.encode())
.offer(
fresh_enr.clone(),
vec![(body_key_2.clone(), body_value_2.encode())],
)
.await
.unwrap();

Expand Down
103 changes: 17 additions & 86 deletions ethportal-peertest/src/scenarios/offer_accept.rs
View file
Open in desktop
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add test here as well?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will remove WireOffer then Offer will take it's place for tests which should be sufficient for test coverage

Original file line number Diff line number Diff line change
Expand Up @@ -22,19 +22,12 @@ pub async fn test_unpopulated_offer(peertest: &Peertest, target: &Client) {
info!("Testing Unpopulated OFFER/ACCEPT flow");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is no longer "unpopulated" flow. If I'm not mistaken, we no longer have "unpopulated" flow.

Either rename/update current test, or remove it if you think it's redundant.


let (content_key, content_value) = fixture_header_by_hash();
// Store content to offer in the testnode db
let store_result = target
.store(content_key.clone(), content_value.encode())
.await
.unwrap();

assert!(store_result);

// Send wire offer request from testnode to bootnode
let result = target
.wire_offer(
.offer(
Enr::from_str(&peertest.bootnode.enr.to_base64()).unwrap(),
vec![content_key.clone()],
vec![(content_key.clone(), content_value.encode())],
)
.await
.unwrap();
Expand All @@ -49,40 +42,14 @@ pub async fn test_unpopulated_offer(peertest: &Peertest, target: &Client) {
);
}

pub async fn test_unpopulated_offer_fails_with_missing_content(
peertest: &Peertest,
target: &Client,
) {
info!("Testing Unpopulated OFFER/ACCEPT flow with missing content");

let (content_key, _content_value) = fixture_header_by_hash();

// validate that wire offer fails if content not available locally
match target
.wire_offer(
Enr::from_str(&peertest.bootnode.enr.to_base64()).unwrap(),
vec![content_key.clone()],
)
.await
{
Ok(_) => panic!("Unpopulated offer should have failed"),
Err(e) => {
assert!(e
.to_string()
.contains("Content key not found in local store"));
}
}
}

pub async fn test_populated_offer(peertest: &Peertest, target: &Client) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Along with @morph-dev comment above, you can update the name of this test & log to just test_offer ... and remove any reference to unpopulated/populated in other test names / logs / comments in this file

info!("Testing Populated Offer/ACCEPT flow");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit Testing OFFER/ACCEPT flow / Testing Offer/Accept flow (& in other test's info! logs)


let (content_key, content_value) = fixture_header_by_hash();
let result = target
.offer(
Enr::from_str(&peertest.bootnode.enr.to_base64()).unwrap(),
content_key.clone(),
content_value.encode(),
vec![(content_key.clone(), content_value.encode())],
)
.await
.unwrap();
Expand Down Expand Up @@ -144,8 +111,7 @@ pub async fn test_offer_propagates_gossip(peertest: &Peertest, target: &Client)
target
.offer(
peertest.bootnode.enr.clone(),
content_key.clone(),
content_value.encode(),
vec![(content_key.clone(), content_value.encode())],
)
.await
.unwrap();
Expand Down Expand Up @@ -178,15 +144,10 @@ pub async fn test_offer_propagates_gossip_with_large_content(peertest: &Peertest
.await
.unwrap();
assert!(store_result);
let store_result = target
.store(body_key.clone(), body_value.encode())
.await
.unwrap();
assert!(store_result);
target
.wire_offer(
.offer(
peertest.bootnode.ipc_client.node_info().await.unwrap().enr,
vec![body_key.clone()],
vec![(body_key.clone(), body_value.encode())],
)
.await
.unwrap();
Expand Down Expand Up @@ -221,8 +182,7 @@ pub async fn test_offer_propagates_gossip_multiple_content_values(
target
.offer(
peertest.bootnode.enr.clone(),
header_key.clone(),
header_value.encode(),
vec![(header_key.clone(), header_value.encode())],
)
.await
.unwrap();
Expand All @@ -241,23 +201,14 @@ pub async fn test_offer_propagates_gossip_multiple_content_values(
wait_for_history_content(&peertest.nodes[0].ipc_client, header_key.clone()).await,
);

// Store content to offer in the testnode db
let store_result = target
.store(body_key.clone(), body_value.encode())
.await
.unwrap();
assert!(store_result);
let store_result = target
.store(receipts_key.clone(), receipts_value.encode())
.await
.unwrap();
assert!(store_result);

// here everythings stored in target
target
.wire_offer(
.offer(
peertest.bootnode.ipc_client.node_info().await.unwrap().enr,
vec![body_key.clone(), receipts_key.clone()],
vec![
(body_key.clone(), body_value.encode()),
(receipts_key.clone(), receipts_value.encode()),
],
)
.await
.unwrap();
Expand Down Expand Up @@ -308,16 +259,6 @@ pub async fn test_offer_propagates_gossip_multiple_large_content_values(
.await
.unwrap();
assert!(store_result);
let store_result = target
.store(body_key_1.clone(), body_value_1.encode())
.await
.unwrap();
assert!(store_result);
let store_result = target
.store(receipts_key_1.clone(), receipts_value_1.encode())
.await
.unwrap();
assert!(store_result);

let (header_key_2, header_value_2) = fixture_header_by_hash_with_proof_15040641();
let (body_key_2, body_value_2) = fixture_block_body_15040641();
Expand All @@ -329,25 +270,15 @@ pub async fn test_offer_propagates_gossip_multiple_large_content_values(
.await
.unwrap();
assert!(store_result);
let store_result = target
.store(body_key_2.clone(), body_value_2.encode())
.await
.unwrap();
assert!(store_result);
let store_result = target
.store(receipts_key_2.clone(), receipts_value_2.encode())
.await
.unwrap();
assert!(store_result);

target
.wire_offer(
.offer(
peertest.bootnode.ipc_client.node_info().await.unwrap().enr,
vec![
body_key_1.clone(),
receipts_key_1.clone(),
body_key_2.clone(),
receipts_key_2.clone(),
(body_key_1.clone(), body_value_1.encode()),
(receipts_key_1.clone(), receipts_value_1.encode()),
(body_key_2.clone(), body_value_2.encode()),
(receipts_key_2.clone(), receipts_value_2.encode()),
],
)
.await
Expand Down
3 changes: 1 addition & 2 deletions ethportal-peertest/src/scenarios/state.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,8 +63,7 @@ async fn test_state_offer(fixture: &StateFixture, target: &Client, peer: &Peerte
StateNetworkApiClient::offer(
target,
peer.enr.clone(),
fixture.key.clone(),
fixture.raw_offer_value.clone(),
vec![(fixture.key.clone(), fixture.raw_offer_value.clone())],
)
.await
.unwrap();
Expand Down
Loading
Loading