Skip to content
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
1 change: 1 addition & 0 deletions crates/chia-protocol/src/full_node_protocol.rs
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ pub struct RequestBlocks {
pub struct RespondBlocks {
start_height: u32,
end_height: u32,
#[chia(max_length = 64)]
blocks: Vec<FullBlock>,
}
Comment thread
arvidn marked this conversation as resolved.

Expand Down
28 changes: 28 additions & 0 deletions crates/chia-protocol/src/wallet_protocol.rs
Original file line number Diff line number Diff line change
Expand Up @@ -72,14 +72,17 @@ pub struct RejectHeaderRequest {
pub struct RequestRemovals {
height: u32,
header_hash: Bytes32,
#[chia(max_length = 30000)]
coin_names: Option<Vec<Bytes32>>,
}
Comment thread
arvidn marked this conversation as resolved.

#[streamable(message)]
pub struct RespondRemovals {
height: u32,
header_hash: Bytes32,
#[chia(max_length = 30000)]
coins: Vec<(Bytes32, Option<Coin>)>,
#[chia(max_length = 30000)]
proofs: Option<Vec<(Bytes32, Bytes)>>,
}

Expand All @@ -93,14 +96,17 @@ pub struct RejectRemovalsRequest {
pub struct RequestAdditions {
height: u32,
header_hash: Option<Bytes32>,
#[chia(max_length = 30000)]
puzzle_hashes: Option<Vec<Bytes32>>,
}

#[streamable(message)]
pub struct RespondAdditions {
height: u32,
header_hash: Bytes32,
#[chia(max_length = 30000)]
coins: Vec<(Bytes32, Vec<Coin>)>,
#[chia(max_length = 30000)]
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Vec size limits may break existing wallet clients

Medium Severity

The 30000/35000 caps on puzzle_hashes/coin_names in RequestRemovals, RequestAdditions, RequestPuzzleState, RespondPuzzleState, and CoinStateUpdate are far below the 1_600_000 used for RegisterForPhUpdates/RegisterForCoinUpdates. Wallets that currently operate with more puzzle hashes per request (each address counts as a hash) will see legitimate messages rejected with SequenceTooLarge after this change.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 8c79e84. Configure here.

proofs: Option<Vec<(Bytes32, Bytes, Option<Bytes>)>>,
}

Expand All @@ -114,6 +120,7 @@ pub struct RejectAdditionsRequest {
pub struct RespondBlockHeaders {
start_height: u32,
end_height: u32,
#[chia(max_length = 64)]
Comment thread
arvidn marked this conversation as resolved.
Outdated
header_blocks: Vec<HeaderBlock>,
}

Expand Down Expand Up @@ -146,32 +153,39 @@ pub struct RejectHeaderBlocks {
pub struct RespondHeaderBlocks {
start_height: u32,
end_height: u32,
#[chia(max_length = 64)]
header_blocks: Vec<HeaderBlock>,
}

#[streamable(message)]
pub struct RegisterForPhUpdates {
#[chia(max_length = 1600000)]
puzzle_hashes: Vec<Bytes32>,
min_height: u32,
}

#[streamable(message)]
pub struct RespondToPhUpdates {
#[chia(max_length = 1600000)]
puzzle_hashes: Vec<Bytes32>,
min_height: u32,
#[chia(max_length = 1600000)]
coin_states: Vec<CoinState>,
}

#[streamable(message)]
pub struct RegisterForCoinUpdates {
#[chia(max_length = 1600000)]
coin_ids: Vec<Bytes32>,
min_height: u32,
}

#[streamable(message)]
pub struct RespondToCoinUpdates {
#[chia(max_length = 1600000)]
coin_ids: Vec<Bytes32>,
min_height: u32,
#[chia(max_length = 1600000)]
coin_states: Vec<CoinState>,
}

Expand All @@ -180,6 +194,7 @@ pub struct CoinStateUpdate {
height: u32,
fork_height: u32,
peak_hash: Bytes32,
#[chia(max_length = 30000)]
items: Vec<CoinState>,
}

Expand All @@ -190,6 +205,7 @@ pub struct RequestChildren {

#[streamable(message)]
pub struct RespondChildren {
#[chia(max_length = 30000)]
coin_states: Vec<CoinState>,
}

Expand Down Expand Up @@ -217,21 +233,25 @@ pub struct RespondFeeEstimates {

#[streamable(message)]
pub struct RequestRemovePuzzleSubscriptions {
#[chia(max_length = 1600000)]
puzzle_hashes: Option<Vec<Bytes32>>,
}

#[streamable(message)]
pub struct RespondRemovePuzzleSubscriptions {
#[chia(max_length = 1600000)]
puzzle_hashes: Vec<Bytes32>,
}

#[streamable(message)]
pub struct RequestRemoveCoinSubscriptions {
#[chia(max_length = 1600000)]
coin_ids: Option<Vec<Bytes32>>,
}

#[streamable(message)]
pub struct RespondRemoveCoinSubscriptions {
#[chia(max_length = 1600000)]
coin_ids: Vec<Bytes32>,
}

Expand All @@ -245,6 +265,7 @@ pub struct CoinStateFilters {

#[streamable(message)]
pub struct RequestPuzzleState {
#[chia(max_length = 32700)]
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If register_for_coin_updates and register_for_ph_updates can allow 1.6 million, so should these. This limit is insufficient for wallets like Sage to function. Many wallets have over this many puzzle hashes to subscribe to (each one is a wallet address).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

the 1.6 million is derived from the 50 MiB message limit, which will not fit more 32 byte hashes.

Does Sage subscribe to all coins in a single request? This number was picked based on CoinStore.MAX_PUZZLE_HASH_BATCH_SIZE, see: https://github.com/Chia-Network/chia-blockchain/blob/main/chia/full_node/full_node_api.py#L1949

The limit is defined here: https://github.com/Chia-Network/chia-blockchain/blob/main/chia/full_node/coin_store.py#L444

It seems like we silently drop any item past this limit.

puzzle_hashes: Vec<Bytes32>,
previous_height: Option<u32>,
header_hash: Bytes32,
Expand All @@ -254,10 +275,12 @@ pub struct RequestPuzzleState {

#[streamable(message)]
pub struct RespondPuzzleState {
#[chia(max_length = 32700)]
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same as above

puzzle_hashes: Vec<Bytes32>,
height: u32,
header_hash: Bytes32,
is_finished: bool,
#[chia(max_length = 32700)]
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same as above

coin_states: Vec<CoinState>,
}

Expand All @@ -268,6 +291,7 @@ pub struct RejectPuzzleState {

#[streamable(message)]
pub struct RequestCoinState {
#[chia(max_length = 32700)]
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same as above

coin_ids: Vec<Bytes32>,
previous_height: Option<u32>,
header_hash: Bytes32,
Expand All @@ -276,7 +300,9 @@ pub struct RequestCoinState {

#[streamable(message)]
pub struct RespondCoinState {
#[chia(max_length = 32700)]
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same as above

coin_ids: Vec<Bytes32>,
#[chia(max_length = 32700)]
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same as above

coin_states: Vec<CoinState>,
}

Expand Down Expand Up @@ -334,11 +360,13 @@ pub struct RemovedMempoolItem {

#[streamable(message)]
pub struct MempoolItemsAdded {
#[chia(max_length = 30000)]
transaction_ids: Vec<Bytes32>,
}

#[streamable(message)]
pub struct MempoolItemsRemoved {
#[chia(max_length = 30000)]
removed_items: Vec<RemovedMempoolItem>,
}

Expand Down
85 changes: 74 additions & 11 deletions crates/chia-traits/src/streamable.rs
Original file line number Diff line number Diff line change
Expand Up @@ -124,19 +124,28 @@ impl<T: Streamable> Streamable for Vec<T> {
}

fn parse<const TRUSTED: bool>(input: &mut Cursor<&[u8]>) -> Result<Self> {
let len = u32::parse::<TRUSTED>(input)?;
parse_vec_with_max_length::<T, TRUSTED>(input, u32::MAX as usize)
}
}

let mut ret = if mem::size_of::<T>() == 0 {
Vec::<T>::new()
} else {
let limit = 2 * 1024 * 1024 / mem::size_of::<T>();
Vec::<T>::with_capacity(std::cmp::min(limit, len as usize))
};
for _ in 0..len {
ret.push(T::parse::<TRUSTED>(input)?);
}
Ok(ret)
pub fn parse_vec_with_max_length<T: Streamable, const TRUSTED: bool>(
input: &mut Cursor<&[u8]>,
max_length: usize,
) -> Result<Vec<T>> {
let len = u32::parse::<TRUSTED>(input)?;
if (len as usize) > max_length {
return Err(Error::SequenceTooLarge);
}
let mut ret = if mem::size_of::<T>() == 0 {
Vec::<T>::new()
} else {
let limit = 2 * 1024 * 1024 / mem::size_of::<T>();
Vec::<T>::with_capacity(std::cmp::min(limit, len as usize))
};
Comment thread
Rigidity marked this conversation as resolved.
for _ in 0..len {
ret.push(T::parse::<TRUSTED>(input)?);
}
Ok(ret)
}
Comment thread
cursor[bot] marked this conversation as resolved.
Comment thread
arvidn marked this conversation as resolved.

impl Streamable for String {
Expand Down Expand Up @@ -784,3 +793,57 @@ fn test_stream_enum() {
assert_eq!(stream::<TestEnum>(&TestEnum::B), &[1_u8]);
assert_eq!(stream::<TestEnum>(&TestEnum::C), &[255_u8]);
}

#[cfg(test)]
#[derive(Streamable, PartialEq, Debug)]
struct TestBoundedVec {
#[chia(max_length = 3)]
items: Vec<u32>,
}

#[test]
fn test_parse_bounded_vec() {
let buf: &[u8] = &[0, 0, 0, 3, 0, 0, 0, 1, 0, 0, 0, 2, 0, 0, 0, 3];
from_bytes::<TestBoundedVec>(
buf,
TestBoundedVec {
items: vec![1, 2, 3],
},
);
}

#[test]
fn test_parse_bounded_vec_too_large() {
let buf: &[u8] = &[0, 0, 0, 4, 0, 0, 0, 1, 0, 0, 0, 2, 0, 0, 0, 3, 0, 0, 0, 4];
from_bytes_fail::<TestBoundedVec>(buf, Error::SequenceTooLarge);
}

#[cfg(test)]
#[derive(Streamable, PartialEq, Debug)]
struct TestBoundedOptionVec {
#[chia(max_length = 2)]
items: Option<Vec<u32>>,
}

#[test]
fn test_parse_bounded_option_vec_none() {
let buf: &[u8] = &[0];
from_bytes::<TestBoundedOptionVec>(buf, TestBoundedOptionVec { items: None });
}

#[test]
fn test_parse_bounded_option_vec_some() {
let buf: &[u8] = &[1, 0, 0, 0, 2, 0, 0, 0, 1, 0, 0, 0, 2];
from_bytes::<TestBoundedOptionVec>(
buf,
TestBoundedOptionVec {
items: Some(vec![1, 2]),
},
);
}

#[test]
fn test_parse_bounded_option_vec_too_large() {
let buf: &[u8] = &[1, 0, 0, 0, 3, 0, 0, 0, 1, 0, 0, 0, 2, 0, 0, 0, 3];
from_bytes_fail::<TestBoundedOptionVec>(buf, Error::SequenceTooLarge);
}
Loading
Loading