fix(mempool): allow internal batch dependencies #549
Conversation
Clippy issue rust-lang/rust-clippy#12281 was fixed in v1.81. We now have MSRV of 1.82 allowing us to remove our workaround. More specifically, #[async_trait] was triggering this lint, and as of 1.81 no longer does.
* refactor: improve `ApplyBlockErrors` variants * docs: update `CHANGELOG.md` * refactor: address review comments
* Update state.rs * docs: Update expect() * chore: Lints
Improve block-producer top-level errors.
| // | ||
| // Map all outcomes to an error, and provide component context. | ||
| let (id, err) = match component_result { | ||
| Ok((id, Ok(_))) => (id, Err(anyhow::anyhow!("Component completed unexpectedly"))), |
There was a problem hiding this comment.
nit: Possibly marginally more useful to output which component completed succesfully here.
There was a problem hiding this comment.
This context gets added lower down, since we first have to map the ID to component name.
| Ok((id, Err(err))) => (id, Err(err)), | ||
| Err(join_err) => (join_err.id(), Err(join_err).context("Joining component task")), | ||
| }; | ||
| let component = component_ids.get(&id).unwrap_or(&"unknown"); |
There was a problem hiding this comment.
Should this error/panic instead of defaulting to "unknown"? Looks like it shouldn't really happen
| Err(err) => err.id(), | ||
| }; | ||
| let task = task_ids.get(&task_id).unwrap_or(&"unknown"); |
There was a problem hiding this comment.
Same comment about panicking instead of defaulting to "unknown"
| @@ -278,6 +282,17 @@ mod tests { | |||
| assert_eq!(err, expected); | |||
| } | |||
|
|
|||
| #[test] | |||
| fn insert_with_internal_parent_succeeds() { | |||
| //! Ensure that a batch with internal dependencies can be inserted. | |||
There was a problem hiding this comment.
nit: this should be a simple comment (i.e., //), right?
| &mut self, | ||
| id: BatchJobId, | ||
| transactions: Vec<TransactionId>, | ||
| parents: BTreeSet<TransactionId>, | ||
| mut parents: BTreeSet<TransactionId>, |
There was a problem hiding this comment.
Could we add an explanation of what parents is to the doc comment for this function? (e.g., we should mention that parents may contain transactions also in transactions).
This PR fixes a crash when a batch contained both a parent and child transaction.
In addition, this PR improves the error presentation when the node crashes.
Background: client integration tests were stalling when run against the new mempool based block-producer. This PR fixes the bug that causes this.
The stall occurred because the block-producer component exited, but the node continued on as it only crashes if a component errors. This PR rectifies this behaviour; we now crash the node if any component ends. In addition, this PR improves the errors by identifying the failing component.
This PR also merges in
next. This was required to properly compare and debug the client integration tests. It might be best to review the last few commits instead.