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

rpc-v2/tx/tests: Add transaction broadcast tests and check propagated tx status #3193

Merged
merged 20 commits into from
Feb 28, 2024

Conversation

lexnv
Copy link
Contributor

@lexnv lexnv commented Feb 2, 2024

This PR adds tests for the transaction_broadcast method.

The testing needs to coordinate the following components:

  • The TestApi marks transactions as invalid and implements ChainApi::validate_transaction
    • this is what dictates if a transaction is valid or not and is called from within the BasicPool
  • The BasicPool which maintains the transactions and implements submit_and_watch needed by the tx broadcast to submit the transaction
    • The status of the transaction pool is exposed by mocking the BasicPool
  • The ChainHeadMockClient which mocks the BlockchainEvents::import_notification_stream needed by the tx broadcast to know to which blocks the transaction is submitted

The following changes have been added to the substrate testing to accommodate this:

  • TestApi gets remove_invalid, counterpart to add_invalid to ensure an invalid transaction can become valid again; as well as a priority setter for extrinsics
  • BasicPool test constructor is extended with options for the PoolRotator
    • this mechanism is needed because transactions are banned for 30mins (default) after they are declared invalid
    • testing bypasses this by providing a Duration::ZERO

Testing Scenarios

  • Capture the status of the transaction as it is normally broadcasted
  • transaction_stop is valid while the transaction is in progress
  • A future transaction is handled when the dependencies are completed
  • Try to resubmit the transaction at a later block (currently invalid)
    • An invalid transaction status is propagated; the transaction is marked as temporarily banned; then the ban expires and transaction is resubmitted

This builds on top of: #3079
Part of: #3084

cc @paritytech/subxt-team

@lexnv lexnv added A1-insubstantial Pull request requires no code review (e.g., a sub-repository hash update). R0-silent Changes should not be mentioned in any release notes T3-RPC_API This PR/Issue is related to RPC APIs. labels Feb 2, 2024
Base automatically changed from lexnv/broadcast-tx to master February 12, 2024 13:50
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
@lexnv lexnv changed the title rpc-v2/tx/tests: Add transaction middleware and check propagated tx status rpc-v2/tx/tests: Add transaction broadcast tests and check propagated tx status Feb 13, 2024
@lexnv lexnv requested a review from skunert February 14, 2024 16:20
/// Get the next event from the provided middleware in at most 60 seconds.
macro_rules! get_next_event {
($middleware:expr) => {
tokio::time::timeout(std::time::Duration::from_secs(60), $middleware.recv())
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we expect to ever get close to 60 seconds here? Can't we fail much faster?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should get the response almost immediately. I'll bring this value down a bit :D

Comment on lines +96 to +120
macro_rules! get_next_event {
($middleware:expr) => {
tokio::time::timeout(std::time::Duration::from_secs(5), $middleware.recv())
.await
.unwrap()
.unwrap()
};
}

/// Collect the next number of transaction events from the provided middleware.
macro_rules! get_next_tx_events {
($middleware:expr, $num:expr) => {{
let mut events = std::collections::HashMap::new();
for _ in 0..$num {
let event = get_next_event!($middleware);
match event {
crate::transaction::tests::middleware_pool::MiddlewarePoolEvent::TransactionStatus { transaction, status } => {
events.entry(transaction).or_insert_with(|| vec![]).push(status);
},
other => panic!("Expected TransactionStatus, received {:?}", other),
};
}
events
}};
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these macros because $middleware might be something other than MiddlewarePoolRecv?

Copy link
Contributor Author

@lexnv lexnv Feb 26, 2024

Choose a reason for hiding this comment

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

This gives a more useful line number on which the panic happened. The alternative of having a function would always result in a panic at line 100 for example, inside the function. We also use this for the pool middleware and executor middleware

Copy link
Contributor

@jsdw jsdw left a comment

Choose a reason for hiding this comment

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

Just a couple of small nits!

I find a couple of the tests quite long and hard to follow (I wonder if it's possible to make them any smaller in general), but I don't know this code very well, and offhand they look useful!

@lexnv lexnv enabled auto-merge February 27, 2024 13:53
@lexnv lexnv added this pull request to the merge queue Feb 28, 2024
Merged via the queue into master with commit f1b2189 Feb 28, 2024
129 of 130 checks passed
@lexnv lexnv deleted the lexnv/broadcast-tx-tests branch February 28, 2024 10:35
ordian added a commit that referenced this pull request Mar 1, 2024
…head-data

* origin/master:
  Fix call enum's metadata regression (#3513)
  Enable elastic scaling node feature in local testnets genesis (#3509)
  update development setup in sdk-docs (#3506)
  Fix accidental no-shows on node restart (#3277)
  Remove `AssignmentProviderConfig` and use parameters from `HostConfiguration` instead (#3181)
  [Deprecation] Remove sp_weights::OldWeight (#3491)
  Fixup multi-collator parachain transition to async backing (#3510)
  Multi-Block-Migrations, `poll` hook and new System callbacks (#1781)
  Snowbridge - Extract Ethereum Chain ID (#3501)
  PVF: re-preparing artifact on failed runtime construction (#3187)
  Add documentation around FRAME Offchain workers (#3463)
  [prdoc] Optional SemVer bumps and Docs (#3441)
  rpc-v2/tx/tests: Add transaction broadcast tests and check propagated tx status (#3193)
ordian added a commit that referenced this pull request Mar 1, 2024
…data

* ao-collator-parent-head-data:
  Fix call enum's metadata regression (#3513)
  Enable elastic scaling node feature in local testnets genesis (#3509)
  update development setup in sdk-docs (#3506)
  Fix accidental no-shows on node restart (#3277)
  Remove `AssignmentProviderConfig` and use parameters from `HostConfiguration` instead (#3181)
  [Deprecation] Remove sp_weights::OldWeight (#3491)
  Fixup multi-collator parachain transition to async backing (#3510)
  Multi-Block-Migrations, `poll` hook and new System callbacks (#1781)
  Snowbridge - Extract Ethereum Chain ID (#3501)
  PVF: re-preparing artifact on failed runtime construction (#3187)
  Add documentation around FRAME Offchain workers (#3463)
  [prdoc] Optional SemVer bumps and Docs (#3441)
  rpc-v2/tx/tests: Add transaction broadcast tests and check propagated tx status (#3193)
bgallois pushed a commit to duniter/duniter-polkadot-sdk that referenced this pull request Mar 25, 2024
… tx status (paritytech#3193)

This PR adds tests for the `transaction_broadcast` method.


The testing needs to coordinate the following components:
- The `TestApi` marks transactions as invalid and implements
`ChainApi::validate_transaction`
- this is what dictates if a transaction is valid or not and is called
from within the `BasicPool`
- The `BasicPool` which maintains the transactions and implements
`submit_and_watch` needed by the tx broadcast to submit the transaction
- The status of the transaction pool is exposed by mocking the BasicPool
- The `ChainHeadMockClient` which mocks the
`BlockchainEvents::import_notification_stream` needed by the tx
broadcast to know to which blocks the transaction is submitted

The following changes have been added to the substrate testing to
accommodate this:
- `TestApi` gets ` remove_invalid`, counterpart to `add_invalid` to
ensure an invalid transaction can become valid again; as well as a
priority setter for extrinsics
- `BasicPool` test constructor is extended with options for the
`PoolRotator`
- this mechanism is needed because transactions are banned for 30mins
(default) after they are declared invalid
  - testing bypasses this by providing a `Duration::ZERO`

### Testing Scenarios

- Capture the status of the transaction as it is normally broadcasted
- `transaction_stop` is valid while the transaction is in progress
- A future transaction is handled when the dependencies are completed
- Try to resubmit the transaction at a later block (currently invalid)
- An invalid transaction status is propagated; the transaction is marked
as temporarily banned; then the ban expires and transaction is
resubmitted
  
This builds on top of:
paritytech#3079
Part of: paritytech#3084

cc @paritytech/subxt-team

---------

Signed-off-by: Alexandru Vasile <[email protected]>
Co-authored-by: James Wilson <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A1-insubstantial Pull request requires no code review (e.g., a sub-repository hash update). R0-silent Changes should not be mentioned in any release notes T3-RPC_API This PR/Issue is related to RPC APIs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants