Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

rpc: Implement transaction RPC API #12328

Merged
merged 24 commits into from
Oct 11, 2022
Merged

Conversation

lexnv
Copy link
Contributor

@lexnv lexnv commented Sep 22, 2022

This PR implements the RPC spec for the transaction methods.

The transaction methods include:

Details

The implementation is adjusting the minimum amount of code to maintain
both the old RPC, while offering support for the new RPC spec.

  • The spec states that the RPC shall return an error object only if decoding of the
    transaction fails [here]. However, the RPC will also produce an error if the number
    of resources hits its limit or the number of active subscriptions exceeds the limit.
    The dropped event should be used instead, but this requires patching the jsonrpsee first.

  • A validated transaction will enter the tx-pool in the ready or future queues.
    Therefore, those events are converted to the validated event without modifications.

  • The tx-pool events: Usurped, Dropped, Invalid are converted to the invalid event.

  • The tx-pool FinalityTimeout is converted to the dropped event:
    "if the block the transaction is included in takes too long to be finalized".

Testing Done

  • Outdated extrinsic
Ok(Object({"error": String("Invalid transaction: Transaction has a bad signature"), "event": String("invalid")}))
  • Invalid extrinsic format
RPC call failed: ErrorObject { code: InvalidParams, message: "0x prefix is missing at line 1 column 3", data: None }
  • Valid extrinsic format with incompatible metadata
Ok(Object({"error": String("Verification error: Runtime error: Execution failed: Runtime panicked: Bad input data provided to validate_transaction: Could not decode `ChargeAssetTxPayment::asset_id`:\n\tunexpected first byte decoding Option\n"), "event": String("invalid")}))
  • Valid extrinsic life cycle
Ok(Object({"event": String("validated")}))
Ok(Object({"block": Object({"hash": String("0x090969a25a0b6d1b094d41364a0e47cbddf5e94b10800077147f05b3c11b138e"), "index": Number(1)}), "event": String("bestChainBlockIncluded")}))
Ok(Object({"block": Object({"hash": String("0x090969a25a0b6d1b094d41364a0e47cbddf5e94b10800077147f05b3c11b138e"), "index": Number(1)}), "event": String("finalized")}))

Next Steps

  • mod tests.rs for transaction API
  • report RPC exceeding resource limits and subscription permits as events
  • optimize sequential events (inBlock 1, inBlock 2, inBock 3) by submitting only the last event (inBlock 3)

Part of #12071.

polkadot companion: paritytech/polkadot#6058

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
To circumvent the fact that serde does not allow mixing
`#[serde(tag = "event")]` with
`#[serde(tag = "event", content = "block")]`
the public facing subscription structure is serialized
and deserialized to an intermmediate representation.

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 added A0-please_review Pull request needs code review. B3-apinoteworthy C1-low PR touches the given topic and has a low impact on builders. labels Sep 22, 2022
@lexnv lexnv self-assigned this Sep 22, 2022
@tomaka
Copy link
Contributor

tomaka commented Sep 22, 2022

I'm not familiar with the code, but all your explanations look good to me! 👍
I'm quite happy with it because the transactions pool is far from being simple.

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.

The dropped event should be used instead, but this requires patching the jsonrpsee first.

Can you elaborate, what needs to be patches in jsonrpsee?

Otherwise the logic looks good to me, just some nitpicks and the missing companion since we use TransactionStatus in other repos too.

client/rpc-spec-v2/src/transaction/event.rs Outdated Show resolved Hide resolved
client/rpc-spec-v2/src/transaction/event.rs Outdated Show resolved Hide resolved
client/rpc-spec-v2/src/transaction/event.rs Outdated Show resolved Hide resolved
client/service/src/builder.rs Outdated Show resolved Hide resolved
client/transaction-pool/api/src/lib.rs Show resolved Hide resolved
client/transaction-pool/src/graph/listener.rs Outdated Show resolved Hide resolved
@niklasad1
Copy link
Member

niklasad1 commented Sep 23, 2022

The spec states that the RPC shall return an error object only if decoding of the
transaction fails [here]. However, the RPC will also produce an error if the number
of resources hits its limit or the number of active subscriptions exceeds the limit.

Yeah but you could disable this on the server as well if needed. That will be like that until we got backpressure :P

The dropped event should be used instead, but this requires patching the jsonrpsee first.

I don't understand this, it should be sent out as an ordinary notification (which you can do via SubscriptionSink::send) and then just drop the SubscriptionSink? However, I see you use the stream API then it's a bit more awkward to do that but can't you just terminate the stream after that occurs?

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>
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 good!

@@ -143,6 +146,8 @@ pub type TransactionFor<P> = <<P as TransactionPool>::Block as BlockT>::Extrinsi
pub type TransactionStatusStreamFor<P> = TransactionStatusStream<TxHash<P>, BlockHash<P>>;
/// Transaction type for a local pool.
pub type LocalTransactionFor<P> = <<P as LocalTransactionPool>::Block as BlockT>::Extrinsic;
/// Transaction's index within the block in which it was included.
pub type TxIndex = usize;
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we can have a specific type for TxIndex which serializes it as a String instead of having to add this as_string attribute on each usize.

It's easy to mess up...

lexnv and others added 2 commits September 28, 2022 15:17
Co-authored-by: Niklas Adolfsson <niklasadolfsson1@gmail.com>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Copy link
Member

@niklasad1 niklasad1 left a comment

Choose a reason for hiding this comment

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

nice job, looks good

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@@ -362,3 +367,52 @@ impl<TPool: LocalTransactionPool> OffchainSubmitTransaction<TPool::Block> for TP
})
}
}

/// Wrapper functions to keep the API backwards compatible over the wire for the old RPC spec.
mod v1_compatible {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice!

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.

Great job! A couple of nitpicks from me but nothing significant offhand.

lexnv and others added 2 commits September 29, 2022 11:56
Co-authored-by: James Wilson <james@jsdw.me>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
Copy link
Contributor Author

lexnv commented Oct 11, 2022

bot merge

@Polkadot-Forum
Copy link

This pull request has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/polkadot-release-analysis/1026/2

wischli added a commit to KILTprotocol/kilt-node that referenced this pull request Dec 6, 2022
## fixes KILTProtocol/ticket#2289 and KILTProtocol/ticket#2296
* Upgrades from Polkadot v0.9.29 to v0.9.32
* Adds missing feature implementations for all tomls (checked via
`subalfred check features` in all crates)
* Actually necessary for application of
paritytech/substrate#10592 (see runtime changes
in
[dd81eac](dd81eac))
* Migrates Democracy, Preimage and Scheduler pallets to use bounded
Calls, see below

## Summary of changes (Polkadot v0.9.30-0.9.32)

* Weights v2 are not fully there yet, but the struct now includes the
[second field for storage
size](paritytech/substrate#12277) (tracking
issue: https://github.com/paritytech/substrate/issues/12176)

### Breaking Changes
* Breaking: Outer enums
(paritytech/substrate#11981)
  * `Origin` --> `RuntimeOrigin`
  * `Call` --> `RuntimeCall`
  * `Event` --> `RuntimeEvent`
* ~Convention seems to be to keep `Event`, `Call`, `Origin` for inner
pallet usage, e.g. `Did::Origin`~ Update: We use `Runtime` prefix
internally as well

### Noteworthy PRs
* paritytech/substrate#12109
* paritytech/substrate#12328
* paritytech/cumulus#1585
* Following the effort of decoupling collators and full relay nodes,
this PR adds the possibility of pointing the collator to an “external”
(non in-process) relay node. This is still considered experimental, and
the **relay node is suggested to run on the same machine than the
collator for the moment**.
* To specify the relay full node rpc: `polkadot-parachain --alice
--collator --relay-chain-rpc-url <rpc-websocket-url>`
* paritytech/substrate#12486
* Before this change only the interpreted WASM executor was included in
per default compilations. Making the compiled executor opt-in, now,
compiled WASM executor is set by default and an opt-out instead. This
could lead to **big performance difference** between using these two, as
more recent versions of the interpreter see a regression in performance.

### Scheduler, Preimage, Democracy Migration

* paritytech/substrate#11649
* Referenda, Democracy, Scheduler and Preimage pallets are all now
bounded in storage access footprint
* Removed the concept of a "hard deadline" or weight-override priority
and no longer guarantees that at least one scheduled item will be
executed per block (since these are both dangerous to parachains which
have a strict need of weight limits). This means you must ensure that
scheduled items are below the MaximumWeight or they will not be
executed.
* Interesting comment:
paritytech/substrate#11649 (comment)

> There is migration code which moves existing proposals and referenda
over to the new format. However IT DOES NOT MIGRATE EVERYTHING:
> 
> * Preimages are **NOT** migrated. Any registered preimages in
Democracy at the time of migration are dropped. Their balance is **NOT
UNRESERVED**.
> * The re-dispatcher used in the old Democracy implementation is
removed. Any proposals scheduled for dispatch by Democracy **WILL NOT
EXECUTE**.
>
> This means you SHOULD ensure that:
> 
> * **the preimage for the runtime upgrade is placed as an imminent
preimage, not with a deposit;**
> * **no other preimages are in place at the time of upgrade;**
> * **there are no other proposals scheduled for dispatch by Democracy
at the time of upgrade.**
> 
> The Democracy pallet will be marked as deprecated immediately once
Referenda is considered production-ready. **ALL TEAMS ARE RECOMMENDED TO
SWITCH SWAY FROM DEMOCRACY PALLET TO REFERENDA/CONVICTION-VOTING PALLETS
ASAP**

#### Result of `try-runtime` against Spiritnet on Friday Nov 18, 2022:
```
2022-11-18 09:27:23.917  INFO                 main runtime::preimage::migration::v1: Migrating 0 images
2022-11-18 09:27:23.917  INFO                 main runtime::scheduler::migration: Trying to migrate 0 agendas...
2022-11-18 09:27:23.917  INFO                 main runtime::scheduler::migration: Migrated 0 agendas.
2022-11-18 09:27:23.917  INFO                 main runtime::democracy::migration::v1: 0 public proposals will be migrated.
2022-11-18 09:27:23.917  INFO                 main runtime::democracy::migration::v1: 25 referenda will be migrated.
2022-11-18 09:27:23.917  INFO                 main runtime::democracy::migration::v1: migrating referendum #7
2022-11-18 09:27:23.917  INFO                 main runtime::democracy::migration::v1: migrating referendum #20
2022-11-18 09:27:23.917  INFO                 main runtime::democracy::migration::v1: migrating referendum #13
2022-11-18 09:27:23.917  INFO                 main runtime::democracy::migration::v1: migrating referendum #5
2022-11-18 09:27:23.917  INFO                 main runtime::democracy::migration::v1: migrating referendum #8
2022-11-18 09:27:23.917  INFO                 main runtime::democracy::migration::v1: migrating referendum #1
2022-11-18 09:27:23.917  INFO                 main runtime::democracy::migration::v1: migrating referendum #19
2022-11-18 09:27:23.917  INFO                 main runtime::democracy::migration::v1: migrating referendum #9
2022-11-18 09:27:23.917  INFO                 main runtime::democracy::migration::v1: migrating referendum #16
2022-11-18 09:27:23.917  INFO                 main runtime::democracy::migration::v1: migrating referendum #14
2022-11-18 09:27:23.917  INFO                 main runtime::democracy::migration::v1: migrating referendum #21
2022-11-18 09:27:23.917  INFO                 main runtime::democracy::migration::v1: migrating referendum #15
2022-11-18 09:27:23.917  INFO                 main runtime::democracy::migration::v1: migrating referendum #24
2022-11-18 09:27:23.917  INFO                 main runtime::democracy::migration::v1: migrating referendum #22
2022-11-18 09:27:23.917  INFO                 main runtime::democracy::migration::v1: migrating referendum #2
2022-11-18 09:27:23.917  INFO                 main runtime::democracy::migration::v1: migrating referendum #10
2022-11-18 09:27:23.917  INFO                 main runtime::democracy::migration::v1: migrating referendum #0
2022-11-18 09:27:23.917  INFO                 main runtime::democracy::migration::v1: migrating referendum #6
2022-11-18 09:27:23.917  INFO                 main runtime::democracy::migration::v1: migrating referendum #11
2022-11-18 09:27:23.917  INFO                 main runtime::democracy::migration::v1: migrating referendum #3
2022-11-18 09:27:23.917  INFO                 main runtime::democracy::migration::v1: migrating referendum #17
2022-11-18 09:27:23.917  INFO                 main runtime::democracy::migration::v1: migrating referendum #18
2022-11-18 09:27:23.917  INFO                 main runtime::democracy::migration::v1: migrating referendum #23
2022-11-18 09:27:23.917  INFO                 main runtime::democracy::migration::v1: migrating referendum #4
2022-11-18 09:27:23.917  INFO                 main runtime::democracy::migration::v1: migrating referendum #25
2022-11-18 09:27:23.918  INFO                 main runtime::democracy::migration::v1: 0 public proposals migrated, 25 referenda migrated
```

## Checklist:

- [x] I have verified that the code works
- [x] No panics! (checked arithmetic ops, no indexing `array[3]` use
`get(3)`, ...)
- [x] I have verified that the code is easy to understand
  - [ ] If not, I have left a well-balanced amount of inline comments
- [x] I have [left the code in a better
state](https://deviq.com/principles/boy-scout-rule)
- [x] I have documented the changes (where applicable)
saraswatpuneet added a commit to frequency-chain/frequency that referenced this pull request Dec 13, 2022
# Goal
The goal of this PR is to upgrade substrate to polkadot release .9.30

Closes #704 

Notes:

[PR for Substrate upgrade to
0.9.30](https://github.com/substrate-developer-hub/substrate-parachain-template/pull/133)
[Release
Notes](https://github.com/paritytech/polkadot/releases/tag/v0.9.30)
# Major updates from 0.9.29 to 0.9.30: 

* Only breaking changes: Rename of Enums in Pallet Config
    ```Call``` -> ```RuntimeCall```
    ```Event``` -> ```RuntimeEvent``` and 
    ```Origin``` -> ```RuntimeOrigin``` . 
Here is the PR paritytech/substrate#11981

* Upgrades from Polkadot v0.9.29 to v0.9.30, version update in
dependencies tree

* Weights update: V2 is slowly getting released and benign updates
carried in this PR. https://github.com/paritytech/substrate/issues/12176
and paritytech/substrate#12277

* @enddynayn @shannonwells some updates in vesting pallet
paritytech/substrate#12109
* @wilwade some pubsub stuff
paritytech/substrate#12328
* @wilwade  paritytech/cumulus#1585 
* @shannonwells some updates in Democracy
paritytech/substrate#11649


# Checklist
- [ ] n/a Chain spec updated
- [ ] n/a Custom RPC OR Runtime API added/changed? Updated
js/api-augment.
- [ ] n/a Design doc(s) updated
- [ ] n/a Tests added
- [ ] n/a Benchmarks added
- [x] Weights updated
- [x] Run benchmarks

Co-authored-by: Dmitri <4452412+demisx@users.noreply.github.com>
Co-authored-by: Jenkins <jenkins@frequency.xyz>
@lexnv lexnv mentioned this pull request Dec 19, 2022
4 tasks
ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
* rpc/tx: Add transaction structures for serialization

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* rpc/tx: Add public facing `TransactionEvent`

To circumvent the fact that serde does not allow mixing
`#[serde(tag = "event")]` with
`#[serde(tag = "event", content = "block")]`
the public facing subscription structure is serialized
and deserialized to an intermmediate representation.

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* rpc/tx: Add trait for the `transaction` API

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* rpc/tx: Convert RPC errors to transaction events

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* rpc/tx: Implement `transaction` RPC methods

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* tx-pool: Propagate tx index to events

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* tx-pool: Adjust testing to reflect tx index in events

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* rpc/tx: Convert tx-pool events for the new RPC spec

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* rpc/tx: Convert tx-pool `FinalityTimeout` event to `Dropped`

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* service: Enable the `transaction` API

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* rpc/tx: Add tests for tx event encoding and decoding

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* tx: Add indentation for subscriptions

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* rpc/tx: Fix documentation

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* rpc/tx: Serialize usize to hex

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* tx-pool: Rename closure parameters

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* service: Separate RPC spec versions

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* rpc/tx: Use `H256` for testing block's hash

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* rpc/tx: Serialize numbers as string

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* tx-pool: Backward compatibility with RPC v1

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* Update client/rpc-spec-v2/src/transaction/transaction.rs

Co-authored-by: Niklas Adolfsson <niklasadolfsson1@gmail.com>

* rpc/tx: Remove comment about serde clone

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* rpc/tx: Use RPC custom error code for invalid tx format

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* Update client/rpc-spec-v2/src/transaction/event.rs

Co-authored-by: James Wilson <james@jsdw.me>

* rpc/tx: Adjust internal structures for serialization/deserialization

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Co-authored-by: Niklas Adolfsson <niklasadolfsson1@gmail.com>
Co-authored-by: James Wilson <james@jsdw.me>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. C1-low PR touches the given topic and has a low impact on builders.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants