-
Notifications
You must be signed in to change notification settings - Fork 666
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.
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
feat: wiring for bandwidth scheduler #12234
Changes from 33 commits
e782412
3e22208
5614b06
a217022
dc156ac
24dd104
c9edcb9
66103e5
47e97a7
40db96e
db96527
db19186
dc1cd5f
d7bfc1e
7c4af68
4336cae
3e63448
caed220
1a27050
54ff731
dfb11b6
272d486
f7ec382
ccec51a
a0ea12d
ca1ebd6
e6bd44e
bb3f370
2af440c
aeea61f
b4fca01
a539c3a
8ad224b
c4fde1a
d8068ee
42eae3b
15336ba
5d921fa
02d8ccb
6241590
8b456a2
aca486c
fa002ab
5a251c9
86cec08
ed32376
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,6 +4,7 @@ use borsh::BorshDeserialize; | |
|
||
use near_crypto::PublicKey; | ||
use near_epoch_manager::EpochManagerAdapter; | ||
use near_primitives::bandwidth_scheduler::BandwidthRequests; | ||
use near_primitives::block::{Block, BlockHeader}; | ||
use near_primitives::challenge::{ | ||
BlockDoubleSign, Challenge, ChallengeBody, ChunkProofs, ChunkState, MaybeEncodedShardChunk, | ||
|
@@ -178,6 +179,10 @@ pub fn validate_chunk_with_chunk_extra_and_receipts_root( | |
} | ||
|
||
validate_congestion_info(&prev_chunk_extra.congestion_info(), &chunk_header.congestion_info())?; | ||
validate_bandwidth_requests( | ||
prev_chunk_extra.bandwidth_requests(), | ||
chunk_header.bandwidth_requests(), | ||
)?; | ||
|
||
Ok(()) | ||
} | ||
|
@@ -211,6 +216,41 @@ fn validate_congestion_info( | |
} | ||
} | ||
|
||
fn validate_bandwidth_requests( | ||
extra_bandwidth_requests: Option<&BandwidthRequests>, | ||
header_bandwidth_requests: Option<&BandwidthRequests>, | ||
) -> Result<(), Error> { | ||
if extra_bandwidth_requests.is_none() | ||
&& header_bandwidth_requests == Some(&BandwidthRequests::empty()) | ||
{ | ||
// This corner case happens for the first chunk that has the BandwidthScheduler feature enabled. | ||
// The previous chunk was applied with a protocol version which doesn't have bandwidth scheduler | ||
// enabled and because of that the bandwidth requests in ChunkExtra are None. | ||
// The header was produced in the new protocol version, and the newer version of header always | ||
// has some bandwidth requests, it's not an `Option`. Because of that the header requests are `Some(BandwidthRequests::empty())`. | ||
return Ok(()); | ||
} | ||
Comment on lines
+223
to
+232
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you do it the same way as it worked for congestion info? Basically they always have to match. I don't remember the exact reasons but it was way cleaner way to handle protocol upgrade this way. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm I'm not sure if it's better, the way congestion control does it causes the protocol to accept two different chunk header versions for one protocol version :/ |
||
|
||
if extra_bandwidth_requests != header_bandwidth_requests { | ||
fn requests_len(requests_opt: Option<&BandwidthRequests>) -> usize { | ||
match requests_opt { | ||
Some(BandwidthRequests::V1(requests_v1)) => requests_v1.requests.len(), | ||
None => 0, | ||
} | ||
} | ||
let error_info_str = format!( | ||
"chunk extra: (is_some: {}, len: {}) chunk header: (is_some: {}, len: {})", | ||
extra_bandwidth_requests.is_some(), | ||
requests_len(extra_bandwidth_requests), | ||
header_bandwidth_requests.is_some(), | ||
requests_len(header_bandwidth_requests) | ||
); | ||
return Err(Error::InvalidBandwidthRequests(error_info_str)); | ||
} | ||
|
||
Ok(()) | ||
} | ||
|
||
/// Validates a double sign challenge. | ||
/// Only valid if ancestors of both blocks are present in the chain. | ||
fn validate_double_sign( | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mini nit: Maybe put it next to congestion control? They are very similar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done