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.

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

test: improve slow tests #3487

Merged
merged 7 commits into from
Jul 2, 2023
Merged
Show file tree
Hide file tree
Changes from 6 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
8 changes: 7 additions & 1 deletion crates/net/eth-wire/src/types/blocks.rs
Original file line number Diff line number Diff line change
Expand Up @@ -68,11 +68,17 @@ impl From<Vec<H256>> for GetBlockBodies {

/// The response to [`GetBlockBodies`], containing the block bodies that the peer knows about if
/// any were found.
#[derive_arbitrary(rlp, 1)]
#[derive_arbitrary(rlp, 16)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

what does the number represent in this derive macro?

Copy link
Member Author

Choose a reason for hiding this comment

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

the number of test cases, default is 256, but we set it to 1 previously because it was expensive. with these changes we can bump it to 16, and while still relatively costly, this is manageable. imo better to have more test cases than a single large test case

#[derive(Clone, Debug, PartialEq, Eq, RlpEncodableWrapper, RlpDecodableWrapper, Default)]
#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
pub struct BlockBodies(
/// The requested block bodies, each of which should correspond to a hash in the request.
#[cfg_attr(
any(test, feature = "arbitrary"),
proptest(
strategy = "proptest::collection::vec(proptest::arbitrary::any::<BlockBody>(), 0..=20)"
)
)]
Comment on lines +76 to +81
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't really understand how proptest works,
why does this need to be specified at all?
I would expect that these things would be configured when the proptest is created, or is this how we pass this information to the macro?

this looks a bit weird at first but I don't really care as long as this speeds up tests

Copy link
Member Author

Choose a reason for hiding this comment

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

the issue is that we are deriving proptest::Arbitrary instead of specifying how it should create these types, so it's very opaque. by default, all vecs have a range of 0..100, so this type in particular by default will have 0..100 block bodies, each with 0..100 ommers, 0..100 transactions (which can have 0..100 access list items, which can have 0..100 storage slots in each item) and so on. as you can see, this quickly explodes.

since the test is auto-generated, we can't configure this in there.

Copy link
Collaborator

Choose a reason for hiding this comment

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

a bit ugly :(, but necessary

pub Vec<BlockBody>,
);

Expand Down
8 changes: 7 additions & 1 deletion crates/net/eth-wire/src/types/receipts.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,17 @@ pub struct GetReceipts(

/// The response to [`GetReceipts`], containing receipt lists that correspond to each block
/// requested.
#[derive_arbitrary(rlp, 1)]
#[derive_arbitrary(rlp)]
#[derive(Clone, Debug, PartialEq, Eq, RlpEncodableWrapper, RlpDecodableWrapper, Default)]
#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
pub struct Receipts(
/// Each receipt hash should correspond to a block hash in the request.
#[cfg_attr(
any(test, feature = "arbitrary"),
proptest(
strategy = "proptest::collection::vec(proptest::collection::vec(proptest::arbitrary::any::<ReceiptWithBloom>(), 0..=50), 0..=5)"
)
)]
pub Vec<Vec<ReceiptWithBloom>>,
);

Expand Down
18 changes: 18 additions & 0 deletions crates/primitives/src/block.rs
Original file line number Diff line number Diff line change
Expand Up @@ -739,10 +739,28 @@ impl From<(BlockHash, BlockNumber)> for BlockNumHash {
#[rlp(trailing)]
pub struct BlockBody {
/// Transactions in the block
#[cfg_attr(
any(test, feature = "arbitrary"),
proptest(
strategy = "proptest::collection::vec(proptest::arbitrary::any::<TransactionSigned>(), 0..=100)"
)
)]
pub transactions: Vec<TransactionSigned>,
/// Uncle headers for the given block
#[cfg_attr(
any(test, feature = "arbitrary"),
proptest(
strategy = "proptest::collection::vec(proptest::arbitrary::any::<Header>(), 0..=2)"
)
)]
pub ommers: Vec<Header>,
/// Withdrawals in the block.
#[cfg_attr(
any(test, feature = "arbitrary"),
proptest(
strategy = "proptest::option::of(proptest::collection::vec(proptest::arbitrary::any::<Withdrawal>(), 0..=16))"
)
)]
pub withdrawals: Option<Vec<Withdrawal>>,
}

Expand Down
6 changes: 6 additions & 0 deletions crates/primitives/src/log.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,12 @@ pub struct Log {
/// Contract that emitted this log.
pub address: Address,
/// Topics of the log. The number of logs depend on what `LOG` opcode is used.
#[cfg_attr(
any(test, feature = "arbitrary"),
proptest(
strategy = "proptest::collection::vec(proptest::arbitrary::any::<H256>(), 0..=5)"
)
)]
pub topics: Vec<H256>,
/// Arbitrary length data.
pub data: Bytes,
Expand Down
6 changes: 6 additions & 0 deletions crates/primitives/src/receipt.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,12 @@ pub struct Receipt {
/// Gas used
pub cumulative_gas_used: u64,
/// Log send from contracts.
#[cfg_attr(
any(test, feature = "arbitrary"),
proptest(
strategy = "proptest::collection::vec(proptest::arbitrary::any::<Log>(), 0..=20)"
)
)]
pub logs: Vec<Log>,
}

Expand Down
19 changes: 16 additions & 3 deletions crates/primitives/src/transaction/access_list.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
use crate::{Address, H256};
use revm_primitives::U256;

use reth_codecs::{main_codec, Compact};
use reth_rlp::{RlpDecodable, RlpDecodableWrapper, RlpEncodable, RlpEncodableWrapper};
use revm_primitives::U256;
use serde::{Deserialize, Serialize};

/// A list of addresses and storage keys that the transaction plans to access.
Expand All @@ -14,13 +13,27 @@ pub struct AccessListItem {
/// Account addresses that would be loaded at the start of execution
pub address: Address,
/// Keys of storage that would be loaded at the start of execution
#[cfg_attr(
any(test, feature = "arbitrary"),
proptest(
strategy = "proptest::collection::vec(proptest::arbitrary::any::<H256>(), 0..=20)"
)
)]
pub storage_keys: Vec<H256>,
}

/// AccessList as defined in EIP-2930
#[main_codec(rlp)]
#[derive(Clone, Debug, PartialEq, Eq, Hash, Default, RlpDecodableWrapper, RlpEncodableWrapper)]
pub struct AccessList(pub Vec<AccessListItem>);
pub struct AccessList(
#[cfg_attr(
any(test, feature = "arbitrary"),
proptest(
strategy = "proptest::collection::vec(proptest::arbitrary::any::<AccessListItem>(), 0..=20)"
)
)]
pub Vec<AccessListItem>,
);

impl AccessList {
/// Converts the list into a vec, expected by revm
Expand Down
5 changes: 3 additions & 2 deletions crates/trie/src/trie.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1212,12 +1212,13 @@ mod tests {
assert_trie_updates(&account_updates);
}

// TODO: limit the thumber of test cases?
proptest! {
#![proptest_config(ProptestConfig {
cases: 128, ..ProptestConfig::default()
})]
#[test]
fn fuzz_state_root_incremental(account_changes: [BTreeMap<H256, U256>; 5]) {
tokio::runtime::Runtime::new().unwrap().block_on(async {

let db = create_test_rw_db();
let factory = ProviderFactory::new(db.as_ref(), MAINNET.clone());
let tx = factory.provider_rw().unwrap();
Expand Down