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: Implement transaction_unstable_broadcast and transaction_unstable_stop #3079

Merged
merged 40 commits into from
Feb 12, 2024

Conversation

lexnv
Copy link
Contributor

@lexnv lexnv commented Jan 26, 2024

This PR implements the transaction_unstable_broadcast and transaction_unstable_stop.

The transaction_unstable_broadcast submits the provided transaction at the best block of the chain.
If the transaction is dropped or declared invalid, the API tries to resubmit the transaction at the next available best block.

Broadcasting

The broadcasting operation continues until either:

  • the user called transaction_unstable_stop with the operation ID that identifies the broadcasting operation
  • the transaction state is one of the following:
    • Finalized: the transaction is part of the chain
    • FinalizedTimeout: we have waited for 256 finalized blocks and timedout
    • Usurped the transaction has been replaced in the tx pool

The broadcasting retires to submit the transaction when the transaction state is:

  • Invalid: the transaction might become valid at a later time
  • Dropped: the transaction pool's capacity is full at the moment, but might clear when other transactions are finalized/dropped

Stopping

The transaction_unstable_broadcast spawns an abortable future and tracks the abort handler.
When the transaction_unstable_stop is called with a valid operation ID; the abort handler of the corresponding transaction_unstable_broadcast future is called. This behavior ensures the broadcast future is finishes on the next polling.
When the transaction_unstable_stop is called with an invalid operation ID, an invalid jsonrpc specific error object is returned.

Testing

This PR adds the testing harness of the transaction API and validates two basic scenarios:

  • transaction enters and exits the transaction pool
  • transaction stop returns appropriate values when called with valid and invalid operation IDs

Closes: #3039

Note that the API should be enabled after: #3084.

cc @paritytech/subxt-team

lexnv added 19 commits January 24, 2024 12:04
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv lexnv self-assigned this Jan 26, 2024
@lexnv lexnv added I5-enhancement An additional feature request. T3-RPC_API This PR/Issue is related to RPC APIs. D1-medium Can be fixed by a coder with good Rust knowledge but little knowledge of the codebase. labels Jan 26, 2024
lexnv added 2 commits January 26, 2024 13:45
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
lexnv added 2 commits February 2, 2024 16:26
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Comment on lines +126 to +136
while !is_done {
// Wait for the last block to become available.
let Some(best_block_hash) =
last_stream_element(&mut best_block_import_stream).await
else {
return;
};

let mut stream = match pool
.submit_and_watch(best_block_hash, TX_SOURCE, decoded_extrinsic.clone())
.await
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 are effectively trying to resubmit the transaction to the next best block here; if we cannot make progress at this time:

  • submit_and_watch directly returned a recoverable error (ImmediatelyDroped etc)
  • submit_and_watch produced an event that may be recoverable (Invalid / Dropped etc)

When we get the Invalid event, the transaction-pool will ban it for some time (default 30 mins). In this case, the looping seems redundant unless chain developers manually pick another timeout.

However, I believe we can simplify this and try to submit the transaction only once.
From the transaction_unstable_broadcast, the phrasing states that the server tries to broadcast, but without any guarantees:

JSON-RPC server will try to propagate this transaction

This could be interpreted as we'll submit the transaction to the transaction pool now, and if it fails or is invalid we'll stop.

@skunert would love to get your thoughts on this 🙏

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah sounds like its not enforced by the spec, but IMO the current implementation makes sense. If we can recover and retry in the next block, why not.

substrate/client/rpc-spec-v2/src/transaction/api.rs Outdated Show resolved Hide resolved
// Keep track of this entry and the abortable handle.
{
let mut broadcast_ids = self.broadcast_ids.write();
broadcast_ids.insert(id.clone(), BroadcastState { handle });
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm could someone abuse broadcast_ids by calling broadcast for invalid tx over and over, but never calling stop_broadcast? The future would terminate but the broadcast ids map would grow and never get cleaned up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! I've removed the operationId from our internal mappings as soon as the broadcast future exits.

I believe this is in-line with the spec; since the tx_stop mentions it works for active broadcasts:

A JSON-RPC error is generated if the operationId doesn't correspond to any active transaction_unstable_broadcast operation.

As a side-effect have added a small task executor wrapper for our testing; just to make sure we don't race between tx_stop and broadcast future clearing the internal state

Comment on lines +126 to +136
while !is_done {
// Wait for the last block to become available.
let Some(best_block_hash) =
last_stream_element(&mut best_block_import_stream).await
else {
return;
};

let mut stream = match pool
.submit_and_watch(best_block_hash, TX_SOURCE, decoded_extrinsic.clone())
.await
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah sounds like its not enforced by the spec, but IMO the current implementation makes sense. If we can recover and retry in the next block, why not.

lexnv and others added 5 commits February 8, 2024 18:05
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
…cast.rs

Co-authored-by: Sebastian Kunert <skunert49@gmail.com>
Copy link
Contributor

@skunert skunert left a comment

Choose a reason for hiding this comment

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

Looks nice!

github-merge-queue bot pushed a commit that referenced this pull request Feb 12, 2024
…lpers (#3215)

This PR improves the transaction status documentation.
- Added doc references for describing the main states
- Extra comment wrt pool ready / future queues
- `FinalityTimeout` no longer describes a lagging finality gadget, it
signals that the maximum number of finality gadgets has been reached

A few helper methods are added to indicate when:
- a final event is generated by the transaction pool for a given event
- a final event is provided, although the transaction might become valid
at a later time and could be re-submitted

The helper methods are used and taken from
#3079 to help us better
keep it in sync.


cc @paritytech/subxt-team

---------

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv lexnv added this pull request to the merge queue Feb 12, 2024
Merged via the queue into master with commit bde0bbe Feb 12, 2024
123 of 125 checks passed
@lexnv lexnv deleted the lexnv/broadcast-tx branch February 12, 2024 13:50
github-merge-queue bot pushed a commit that referenced this pull request Feb 28, 2024
… tx status (#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:
#3079
Part of: #3084

cc @paritytech/subxt-team

---------

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Co-authored-by: James Wilson <james@jsdw.me>
bgallois pushed a commit to duniter/duniter-polkadot-sdk that referenced this pull request Mar 25, 2024
…lpers (paritytech#3215)

This PR improves the transaction status documentation.
- Added doc references for describing the main states
- Extra comment wrt pool ready / future queues
- `FinalityTimeout` no longer describes a lagging finality gadget, it
signals that the maximum number of finality gadgets has been reached

A few helper methods are added to indicate when:
- a final event is generated by the transaction pool for a given event
- a final event is provided, although the transaction might become valid
at a later time and could be re-submitted

The helper methods are used and taken from
paritytech#3079 to help us better
keep it in sync.


cc @paritytech/subxt-team

---------

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
bgallois pushed a commit to duniter/duniter-polkadot-sdk that referenced this pull request Mar 25, 2024
…n_unstable_stop` (paritytech#3079)

This PR implements the
[transaction_unstable_broadcast](https://github.com/paritytech/json-rpc-interface-spec/blob/main/src/api/transaction_unstable_broadcast.md)
and
[transaction_unstable_stop](https://github.com/paritytech/json-rpc-interface-spec/blob/main/src/api/transaction_unstable_stop.md).


The
[transaction_unstable_broadcast](https://github.com/paritytech/json-rpc-interface-spec/blob/main/src/api/transaction_unstable_broadcast.md)
submits the provided transaction at the best block of the chain.
If the transaction is dropped or declared invalid, the API tries to
resubmit the transaction at the next available best block.

### Broadcasting 
The broadcasting operation continues until either:

- the user called `transaction_unstable_stop` with the operation ID that
identifies the broadcasting operation
- the transaction state is one of the following: 
  - Finalized: the transaction is part of the chain
- FinalizedTimeout: we have waited for 256 finalized blocks and timedout
  - Usurped the transaction has been replaced in the tx pool
  
The broadcasting retires to submit the transaction when the transaction
state is:
- Invalid: the transaction might become valid at a later time
- Dropped: the transaction pool's capacity is full at the moment, but
might clear when other transactions are finalized/dropped

### Stopping

The `transaction_unstable_broadcast` spawns an abortable future and
tracks the abort handler.
When the
[transaction_unstable_stop](https://github.com/paritytech/json-rpc-interface-spec/blob/main/src/api/transaction_unstable_stop.md)
is called with a valid operation ID; the abort handler of the
corresponding `transaction_unstable_broadcast` future is called. This
behavior ensures the broadcast future is finishes on the next polling.
When the `transaction_unstable_stop` is called with an invalid operation
ID, an invalid jsonrpc specific error object is returned.


### Testing

This PR adds the testing harness of the transaction API and validates
two basic scenarios:
- transaction enters and exits the transaction pool
- transaction stop returns appropriate values when called with valid and
invalid operation IDs


Closes: paritytech#3039

Note that the API should be enabled after:
paritytech#3084.

cc @paritytech/subxt-team

---------

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Co-authored-by: Sebastian Kunert <skunert49@gmail.com>
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 <alexandru.vasile@parity.io>
Co-authored-by: James Wilson <james@jsdw.me>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
D1-medium Can be fixed by a coder with good Rust knowledge but little knowledge of the codebase. I5-enhancement An additional feature request. T3-RPC_API This PR/Issue is related to RPC APIs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[RPC-Spec-V2] transaction: Implement transaction_unstable_broadcast
4 participants