test: check the tx submission protocol in the simulation#728
Draft
etorreborre wants to merge 1 commit intomainfrom
Draft
test: check the tx submission protocol in the simulation#728etorreborre wants to merge 1 commit intomainfrom
etorreborre wants to merge 1 commit intomainfrom
Conversation
Contributor
WalkthroughAdds deterministic proptest transaction generators, threads generated transactions through the simulator, wires a tx-inject stage that inserts transactions into in-simulation mempools, converts test configs to event streams (peer actions or injected txs), and adds a tx-submission check that verifies expected TxIds are present on nodes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant TestRunner as "Test Runner"
participant Node as "Node (actions_stage)"
participant TxInject as "tx_inject stage"
participant Mempool as "MemoryPool"
Note over TestRunner,Node: Simulation processes events queue
TestRunner->>Node: enqueue SimulationEvent::PeerAction(Action)
Node->>Node: route to actions_stage
TestRunner->>TxInject: enqueue SimulationEvent::InjectTx(Transaction)
TxInject->>Mempool: insert(Transaction, TxOrigin::Local)
Mempool-->>TxInject: Result (ok / err)
TxInject->>TestRunner: log success/warn
rect rgba(30,144,255,0.5)
Note over Node,TxInject: Per-node stages
end
rect rgba(34,139,34,0.5)
Note over Mempool: In-sim resource (ResourceMempool)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
etorreborre
force-pushed
the
etorreborre/test/check-tx-submission-in-simulation
branch
from
4d3212b to
c37eff5
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
simulation/amaru-sim/src/simulator/run_tests.rs (1)
113-114: Decide whether the tx count is global or per downstream.Right now every downstream gets the full
transactionsslice, sonumber_of_transactions = 10with 3 downstream peers turns into 30 submissions, andexpected_tx_idsrepeats the same ids three times. If that’s intentional, a quick rename/comment would save future head-scratching; otherwise split the list across peers or dedupe before validation.Also applies to: 167-176
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@simulation/amaru-sim/src/simulator/run_tests.rs` around lines 113 - 114, The test currently treats transactions as global by passing the entire transactions slice to every downstream (node_configs.iter().flat_map(|config| config.transaction_ids()).collect::<Vec<_>>() and then check_properties(nodes, actions, &expected_tx_ids)), which causes each downstream to submit the same transactions multiple times; decide whether you want per-peer partitioning or global dedupe: if global is intended, add a clarifying comment/rename expected_tx_ids to expected_tx_ids_global and dedupe before validation (e.g., collect unique ids) so check_properties validates uniqueness; if per-peer is intended, change the construction of expected_tx_ids to partition node_configs.transaction_ids() across peers (split the transactions slice among downstream peers) so each downstream only gets its share and update check_properties inputs accordingly.crates/amaru-consensus/src/headers_tree/data_generation/actions.rs (1)
541-546:nbis only an upper bound right now.The post-filter drops duplicate
TxIds, so this strategy can return fewer transactions than requested. That’s a wee footgun for a knob callednumber_of_transactions; I’d rather reject-and-regenerate here so the generator actually honours the requested count.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/amaru-consensus/src/headers_tree/data_generation/actions.rs` around lines 541 - 546, The current any_transactions function post-filters duplicates, which can return fewer than nb items; instead reject-and-regenerate by ensuring the generated vector already has nb unique TxIds: replace the prop_map dedupe with a prop_filter (or prop_filter_map) that checks the produced Vec<Transaction> has size nb and that all TxId::from(&tx) values are unique (e.g., build a BTreeSet of TxId and require its length == nb as usize). Keep any_transaction() as the element strategy and use the predicate referencing TxId to force regeneration when duplicates occur.crates/amaru/src/tests/setup.rs (1)
50-50: Consider using&[NodeTestConfig]instead of&Vec<NodeTestConfig>.Wee nitpick here, mate—taking a slice reference is more idiomatic and flexible since it accepts both
&Vec<T>and&[T]at call sites. It's like ordering "any coffee" instead of "specifically a flat white in a ceramic cup from that shelf."♻️ Slice reference instead of Vec reference
-pub fn create_nodes(rng: &mut RandStdRng, configs: &Vec<NodeTestConfig>) -> anyhow::Result<Nodes> { +pub fn create_nodes(rng: &mut RandStdRng, configs: &[NodeTestConfig]) -> anyhow::Result<Nodes> {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/amaru/src/tests/setup.rs` at line 50, The function signature for create_nodes currently takes an owned Vec reference (&Vec<NodeTestConfig>), which is less idiomatic and less flexible; change the parameter type to a slice reference (&[NodeTestConfig]) in create_nodes and update any call sites accordingly so it accepts both &Vec<NodeTestConfig> and slices, preserving behavior of iterating/accessing NodeTestConfig items inside the function (keep uses of the parameter unchanged, only change its type).crates/amaru/src/tests/configuration.rs (1)
292-303: This works grand, but could be more idiomatic Rust.The ol' for-loop-with-if-let is perfectly functional, but iterator combinators would be a wee bit more elegant—like using a proper espresso machine instead of instant coffee.
♻️ Idiomatic iterator version
pub fn transaction_ids(&self) -> Vec<TxId> { - let mut result = vec![]; - for event in self.events.iter() { - if let SimulationEvent::InjectTx(tx) = event { - result.push(TxId::from(&tx)) - } - } - result + self.events + .iter() + .filter_map(|event| match event { + SimulationEvent::InjectTx(tx) => Some(TxId::from(tx)), + _ => None, + }) + .collect() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/amaru/src/tests/configuration.rs` around lines 292 - 303, Replace the manual for-loop in transaction_ids with an iterator chain: iterate over self.events, use filter_map or flat_map to extract only SimulationEvent::InjectTx variants, convert each extracted tx into a TxId using TxId::from(&tx), and collect() into a Vec<TxId>; this keeps the same behavior but is more idiomatic and concise while still referencing the SimulationEvent::InjectTx pattern and TxId::from conversion.simulation/amaru-sim/src/simulator/checks.rs (1)
109-122: Consider enriching the error message for easier debugging.When the assertion fails, knowing which transactions are missing would be a lifesaver—like having a quest marker instead of wandering the Witcher 3 map blind. Currently, you only report the count mismatch implicitly.
🛠️ Optional: More detailed error message
fn assert_has_all_txs(node: &Node, expected_tx_ids: &[TxId]) -> anyhow::Result<()> { let mempool = node .resources() .get::<ResourceMempool<Transaction>>() .expect("A ResourceMempool<Transaction> must be available") .clone(); let txs = mempool.get_txs_for_ids(expected_tx_ids); if txs.len() != expected_tx_ids.len() { - return Err(anyhow!("node {} did not receive all expected transactions", node.node_id())); + return Err(anyhow!( + "node {} received {}/{} expected transactions", + node.node_id(), + txs.len(), + expected_tx_ids.len() + )); } Ok(()) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@simulation/amaru-sim/src/simulator/checks.rs` around lines 109 - 122, In assert_has_all_txs, improve the error message to list which expected TxIds are missing: after calling mempool.get_txs_for_ids(expected_tx_ids) compute the set difference between expected_tx_ids and the returned txs (using their ids), format the missing ids into the error, and return an anyhow! that includes node.node_id() plus the missing TxIds; reference the ResourceMempool<Transaction>, get_txs_for_ids, and node.node_id() symbols to locate and update the failure branch.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/amaru-consensus/src/headers_tree/data_generation/shrink.rs`:
- Around line 100-112: The tuple Shrinkable impl treats len() as the sum of both
halves but calls complement(from, to) with the same global indices for each
element, causing out-of-range complements and incorrect shrinking; fix by
translating the global from/to into per-element ranges before calling each
element's complement: compute left_len = self.0.len(), call
self.0.complement(from, min(to, left_len)) for the left half, and for the right
half call self.1.complement(max(0, from.saturating_sub(left_len)),
to.saturating_sub(left_len)) (or equivalent index clamping), ensuring indices
are clamped to each element’s length so Vec::complement and GeneratedActions
behave correctly during ddmin.
In `@simulation/amaru-sim/src/simulator/run_tests.rs`:
- Around line 89-91: The test currently only persists the generated actions
fixture, but failures depend on both actions and transactions; update the test
to also persist the transactions returned by generate_transactions by passing
both `actions` and `transactions` into `persist_generated_data` (and any
callers), and update the `persist_generated_data` function signature and
serialization logic in `simulation/amaru-sim/src/simulator/report.rs` to accept
and serialize the `transactions` parameter so saved fixtures contain the full
`(actions, transactions)` input pair for replay and shrinking.
---
Nitpick comments:
In `@crates/amaru-consensus/src/headers_tree/data_generation/actions.rs`:
- Around line 541-546: The current any_transactions function post-filters
duplicates, which can return fewer than nb items; instead reject-and-regenerate
by ensuring the generated vector already has nb unique TxIds: replace the
prop_map dedupe with a prop_filter (or prop_filter_map) that checks the produced
Vec<Transaction> has size nb and that all TxId::from(&tx) values are unique
(e.g., build a BTreeSet of TxId and require its length == nb as usize). Keep
any_transaction() as the element strategy and use the predicate referencing TxId
to force regeneration when duplicates occur.
In `@crates/amaru/src/tests/configuration.rs`:
- Around line 292-303: Replace the manual for-loop in transaction_ids with an
iterator chain: iterate over self.events, use filter_map or flat_map to extract
only SimulationEvent::InjectTx variants, convert each extracted tx into a TxId
using TxId::from(&tx), and collect() into a Vec<TxId>; this keeps the same
behavior but is more idiomatic and concise while still referencing the
SimulationEvent::InjectTx pattern and TxId::from conversion.
In `@crates/amaru/src/tests/setup.rs`:
- Line 50: The function signature for create_nodes currently takes an owned Vec
reference (&Vec<NodeTestConfig>), which is less idiomatic and less flexible;
change the parameter type to a slice reference (&[NodeTestConfig]) in
create_nodes and update any call sites accordingly so it accepts both
&Vec<NodeTestConfig> and slices, preserving behavior of iterating/accessing
NodeTestConfig items inside the function (keep uses of the parameter unchanged,
only change its type).
In `@simulation/amaru-sim/src/simulator/checks.rs`:
- Around line 109-122: In assert_has_all_txs, improve the error message to list
which expected TxIds are missing: after calling
mempool.get_txs_for_ids(expected_tx_ids) compute the set difference between
expected_tx_ids and the returned txs (using their ids), format the missing ids
into the error, and return an anyhow! that includes node.node_id() plus the
missing TxIds; reference the ResourceMempool<Transaction>, get_txs_for_ids, and
node.node_id() symbols to locate and update the failure branch.
In `@simulation/amaru-sim/src/simulator/run_tests.rs`:
- Around line 113-114: The test currently treats transactions as global by
passing the entire transactions slice to every downstream
(node_configs.iter().flat_map(|config|
config.transaction_ids()).collect::<Vec<_>>() and then check_properties(nodes,
actions, &expected_tx_ids)), which causes each downstream to submit the same
transactions multiple times; decide whether you want per-peer partitioning or
global dedupe: if global is intended, add a clarifying comment/rename
expected_tx_ids to expected_tx_ids_global and dedupe before validation (e.g.,
collect unique ids) so check_properties validates uniqueness; if per-peer is
intended, change the construction of expected_tx_ids to partition
node_configs.transaction_ids() across peers (split the transactions slice among
downstream peers) so each downstream only gets its share and update
check_properties inputs accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e7e185a6-28eb-4999-a3c3-96a9971cbb58
📒 Files selected for processing (11)
crates/amaru-consensus/src/headers_tree/data_generation/actions.rscrates/amaru-consensus/src/headers_tree/data_generation/shrink.rscrates/amaru/src/tests/configuration.rscrates/amaru/src/tests/node.rscrates/amaru/src/tests/nodes.rscrates/amaru/src/tests/setup.rssimulation/amaru-sim/src/simulator/args.rssimulation/amaru-sim/src/simulator/checks.rssimulation/amaru-sim/src/simulator/data_generation/generate.rssimulation/amaru-sim/src/simulator/run_config.rssimulation/amaru-sim/src/simulator/run_tests.rs
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
simulation/amaru-sim/src/simulator/checks.rs (1)
109-119: Error message could use a wee bit more detail, like a proper quest log.When transactions are missing, knowing which ones would help debugging heaps. Right now you just get "did not receive all expected transactions" – bit like a game saying "you failed" without telling you why.
🔍 Optional: More informative error diagnostics
fn assert_has_all_txs(node: &Node, expected_tx_ids: &[TxId]) -> anyhow::Result<()> { let mempool = node .resources() .get::<ResourceMempool<Transaction>>() .expect("A ResourceMempool<Transaction> must be available") .clone(); let txs = mempool.get_txs_for_ids(expected_tx_ids); if txs.len() != expected_tx_ids.len() { - return Err(anyhow!("node {} did not receive all expected transactions", node.node_id())); + let received_ids: std::collections::BTreeSet<_> = txs.iter().map(TxId::from).collect(); + let missing: Vec<_> = expected_tx_ids.iter().filter(|id| !received_ids.contains(id)).collect(); + return Err(anyhow!( + "node {} missing {}/{} transactions: {:?}", + node.node_id(), + missing.len(), + expected_tx_ids.len(), + missing + )); } Ok(()) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@simulation/amaru-sim/src/simulator/checks.rs` around lines 109 - 119, The assert_has_all_txs function currently returns a generic error when not all expected transactions are present; update it to compute which TxIds are missing by comparing expected_tx_ids with the ids present in the returned txs (use the mempool.get_txs_for_ids result and TxId type), and include those missing ids (and optionally the expected list length vs actual) in the anyhow! error message along with node.node_id() so the error reads something like "node X missing txs: [id1, id2] (expected N, got M)". Ensure you reference assert_has_all_txs, mempool, get_txs_for_ids and TxId when locating and changing the code.crates/amaru-consensus/src/headers_tree/data_generation/actions.rs (1)
540-547: Heads up, mate: filtering after generation may yield fewer thannbtransactions.The strategy generates
nbtransactions, then filters for uniqueness. If duplicates sneak in (not super likely with 32 random bytes, but still), callers expecting exactlynbtransactions might get fewer. Like ordering a dozen donuts and getting 11 – bit of a bummer, yeah?If you need an exact count, consider using
prop::sample::subsequenceor regenerating until the set hasnbunique entries. But honestly, for simulation testing where you're not banking on an exact number, this is probably grand.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/amaru-consensus/src/headers_tree/data_generation/actions.rs` around lines 540 - 547, The current any_transactions produces nb candidates then filters duplicates with BTreeSet (using TxId::from(&tx)), which can yield fewer than nb items; change the strategy so it guarantees exactly nb unique transactions by either (A) using proptest::sample::subsequence over a sufficiently large pool derived from any_transaction() to pick nb distinct elements, or (B) make any_transactions use prop::collection::vec(any_transaction(), larger_size).prop_filter(...) or prop_flat_map to reject/regenerate vectors until BTreeSet::len() == nb as usize; update references to any_transactions and keep the uniqueness check using TxId to ensure correctness.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crates/amaru-consensus/src/headers_tree/data_generation/actions.rs`:
- Around line 540-547: The current any_transactions produces nb candidates then
filters duplicates with BTreeSet (using TxId::from(&tx)), which can yield fewer
than nb items; change the strategy so it guarantees exactly nb unique
transactions by either (A) using proptest::sample::subsequence over a
sufficiently large pool derived from any_transaction() to pick nb distinct
elements, or (B) make any_transactions use
prop::collection::vec(any_transaction(), larger_size).prop_filter(...) or
prop_flat_map to reject/regenerate vectors until BTreeSet::len() == nb as usize;
update references to any_transactions and keep the uniqueness check using TxId
to ensure correctness.
In `@simulation/amaru-sim/src/simulator/checks.rs`:
- Around line 109-119: The assert_has_all_txs function currently returns a
generic error when not all expected transactions are present; update it to
compute which TxIds are missing by comparing expected_tx_ids with the ids
present in the returned txs (use the mempool.get_txs_for_ids result and TxId
type), and include those missing ids (and optionally the expected list length vs
actual) in the anyhow! error message along with node.node_id() so the error
reads something like "node X missing txs: [id1, id2] (expected N, got M)".
Ensure you reference assert_has_all_txs, mempool, get_txs_for_ids and TxId when
locating and changing the code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c32f3f7a-c08d-4376-9041-063855208f66
📒 Files selected for processing (13)
crates/amaru-consensus/src/headers_tree/data_generation/actions.rscrates/amaru-consensus/src/headers_tree/data_generation/shrink.rscrates/amaru/src/tests/configuration.rscrates/amaru/src/tests/node.rscrates/amaru/src/tests/nodes.rscrates/amaru/src/tests/setup.rscrates/amaru/src/tests/test_cases.rssimulation/amaru-sim/src/simulator/args.rssimulation/amaru-sim/src/simulator/checks.rssimulation/amaru-sim/src/simulator/data_generation/generate.rssimulation/amaru-sim/src/simulator/run_config.rssimulation/amaru-sim/src/simulator/run_tests.rssimulation/amaru-sim/tests/traces_test.rs
✅ Files skipped from review due to trivial changes (2)
- simulation/amaru-sim/tests/traces_test.rs
- crates/amaru/src/tests/nodes.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- simulation/amaru-sim/src/simulator/data_generation/generate.rs
- crates/amaru-consensus/src/headers_tree/data_generation/shrink.rs
- simulation/amaru-sim/src/simulator/run_config.rs
Signed-off-by: Eric Torreborre <etorreborre@yahoo.com>
etorreborre
force-pushed
the
etorreborre/test/check-tx-submission-in-simulation
branch
from
c37eff5 to
183c7da
Compare
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
simulation/amaru-sim/src/simulator/run_tests.rs (1)
165-177:⚠️ Potential issue | 🟡 MinorHeads up: potential duplicate TxIds when multiple downstream nodes exist
G'day mate, I'm seeing something that might trip you up like a Souls boss's second phase. Each downstream node gets the same
transactionsslice (line 173), so when you computeexpected_tx_idsby flat-mapping over all node configs (line 111), you'll get duplicateTxIds ifnumber_of_downstream_peers > 1.In
check_tx_submission_property, you comparetxs.len() != expected_tx_ids.len(), butget_txs_for_idsreturns unique transactions. So with 2 downstream nodes and 10 transactions, you'd have 20 expected IDs but only 10 in the mempool.Currently the default is 1 downstream peer, so this might not bite you yet, but it's a lurking gremlin.
🛠️ Possible fix: deduplicate expected_tx_ids
- let expected_tx_ids = node_configs.iter().flat_map(|config| config.transaction_ids()).collect::<Vec<_>>(); + let expected_tx_ids = node_configs + .iter() + .flat_map(|config| config.transaction_ids()) + .collect::<std::collections::HashSet<_>>() + .into_iter() + .collect::<Vec<_>>();Or alternatively, only collect from downstream nodes once since they all have the same transactions.
Also applies to: 110-112
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@simulation/amaru-sim/src/simulator/run_tests.rs` around lines 165 - 177, The downstream_nodes block constructs multiple NodeTestConfig instances each embedding the same transactions slice, causing duplicate TxIds when you later compute expected_tx_ids (used in check_tx_submission_property) and compare to get_txs_for_ids which returns unique txs; fix by deduplicating expected_tx_ids before the length/containment checks or by changing the logic that builds expected_tx_ids to only collect transactions once (e.g., collect from the first downstream NodeTestConfig or dedupe via a HashSet) so expected_tx_ids matches the unique set returned by get_txs_for_ids; update references in the expected_tx_ids computation and any checks in check_tx_submission_property to use the deduplicated set.
🧹 Nitpick comments (2)
simulation/amaru-sim/src/simulator/checks.rs (1)
109-122: Logic is sound, but the error message could use more intelThe assertion correctly leverages
get_txs_for_idsreturning only existing transactions to detect missing ones via length comparison. Proper clever, that.However, when this fails, you'll be like "wait, WHICH transactions are missing?" - a bit of a Dark Souls moment where you die but don't know what hit ya. Consider logging or including the missing IDs in the error for faster debugging.
♻️ Optional: More informative error message
fn assert_has_all_txs(node: &Node, expected_tx_ids: &[TxId]) -> anyhow::Result<()> { let mempool = node .resources() .get::<ResourceMempool<Transaction>>() .expect("A ResourceMempool<Transaction> must be available") .clone(); let txs = mempool.get_txs_for_ids(expected_tx_ids); if txs.len() != expected_tx_ids.len() { - return Err(anyhow!("node {} did not receive all expected transactions", node.node_id())); + let received_ids: std::collections::HashSet<_> = txs.iter().map(TxId::from).collect(); + let missing: Vec<_> = expected_tx_ids + .iter() + .filter(|id| !received_ids.contains(id)) + .collect(); + return Err(anyhow!( + "node {} missing {} of {} expected transactions: {:?}", + node.node_id(), + missing.len(), + expected_tx_ids.len(), + missing + )); } Ok(()) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@simulation/amaru-sim/src/simulator/checks.rs` around lines 109 - 122, In assert_has_all_txs, enhance the error when mempool.get_txs_for_ids(expected_tx_ids) returns fewer entries by computing which TxIds are missing and including them (and optionally the found IDs) in the anyhow! error message; update the error construction in assert_has_all_txs (which uses Node, ResourceMempool<Transaction>, get_txs_for_ids, and node.node_id()) to list missing IDs so failures show exactly which expected transactions were not received.crates/amaru/src/tests/configuration.rs (1)
293-303: Solid implementation, though could be more "Rust-y"The logic is spot-on, mate. If you fancy a more idiomatic approach (like going from PS2 graphics to PS5), you could use
filter_map:♻️ Optional: Iterator-based approach
pub fn transaction_ids(&self) -> Vec<TxId> { - let mut result = vec![]; - for event in self.events.iter() { - if let SimulationEvent::InjectTx(tx) = event { - result.push(TxId::from(&tx)) - } - } - result + self.events + .iter() + .filter_map(|event| match event { + SimulationEvent::InjectTx(tx) => Some(TxId::from(tx)), + _ => None, + }) + .collect() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/amaru/src/tests/configuration.rs` around lines 293 - 303, The transaction_ids method can be made more idiomatic by replacing the explicit loop with an iterator chain: use self.events.iter().filter_map(|event| matches!(event, SimulationEvent::InjectTx(tx) => Some(TxId::from(tx)) /* or ref pattern */)).collect() to produce Vec<TxId>; update the pattern to match the reference type used in SimulationEvent::InjectTx so TxId::from receives the correct arg.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@simulation/amaru-sim/src/simulator/run_tests.rs`:
- Around line 165-177: The downstream_nodes block constructs multiple
NodeTestConfig instances each embedding the same transactions slice, causing
duplicate TxIds when you later compute expected_tx_ids (used in
check_tx_submission_property) and compare to get_txs_for_ids which returns
unique txs; fix by deduplicating expected_tx_ids before the length/containment
checks or by changing the logic that builds expected_tx_ids to only collect
transactions once (e.g., collect from the first downstream NodeTestConfig or
dedupe via a HashSet) so expected_tx_ids matches the unique set returned by
get_txs_for_ids; update references in the expected_tx_ids computation and any
checks in check_tx_submission_property to use the deduplicated set.
---
Nitpick comments:
In `@crates/amaru/src/tests/configuration.rs`:
- Around line 293-303: The transaction_ids method can be made more idiomatic by
replacing the explicit loop with an iterator chain: use
self.events.iter().filter_map(|event| matches!(event,
SimulationEvent::InjectTx(tx) => Some(TxId::from(tx)) /* or ref pattern
*/)).collect() to produce Vec<TxId>; update the pattern to match the reference
type used in SimulationEvent::InjectTx so TxId::from receives the correct arg.
In `@simulation/amaru-sim/src/simulator/checks.rs`:
- Around line 109-122: In assert_has_all_txs, enhance the error when
mempool.get_txs_for_ids(expected_tx_ids) returns fewer entries by computing
which TxIds are missing and including them (and optionally the found IDs) in the
anyhow! error message; update the error construction in assert_has_all_txs
(which uses Node, ResourceMempool<Transaction>, get_txs_for_ids, and
node.node_id()) to list missing IDs so failures show exactly which expected
transactions were not received.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5bbe7a00-10f2-4a19-b532-269fefcc6b17
📒 Files selected for processing (14)
crates/amaru-consensus/src/headers_tree/data_generation/actions.rscrates/amaru-consensus/src/headers_tree/data_generation/shrink.rscrates/amaru/src/tests/configuration.rscrates/amaru/src/tests/node.rscrates/amaru/src/tests/nodes.rscrates/amaru/src/tests/setup.rscrates/amaru/src/tests/test_cases.rssimulation/amaru-sim/src/simulator/args.rssimulation/amaru-sim/src/simulator/checks.rssimulation/amaru-sim/src/simulator/data_generation/generate.rssimulation/amaru-sim/src/simulator/report.rssimulation/amaru-sim/src/simulator/run_config.rssimulation/amaru-sim/src/simulator/run_tests.rssimulation/amaru-sim/tests/traces_test.rs
✅ Files skipped from review due to trivial changes (2)
- crates/amaru/src/tests/nodes.rs
- crates/amaru/src/tests/test_cases.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- simulation/amaru-sim/tests/traces_test.rs
- simulation/amaru-sim/src/simulator/data_generation/generate.rs
- crates/amaru/src/tests/node.rs
- crates/amaru-consensus/src/headers_tree/data_generation/actions.rs
- crates/amaru/src/tests/setup.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR extends the current simulation test with the generation of transactions in downstream nodes and the verification that those transactions eventually end-up in upstream nodes.
Summary by CodeRabbit
New Features
UX / Configuration