jsonrpc: add finality to tx requests (`tx`, `EXPERIMENTAL_tx_status`), add method `broadcast_tx` #9644

telezhnaya · 2023-10-05T13:24:44Z

I'll need to update the docs and jsonrpc_client (and maybe something else, I haven't already checked)

Important updates:

We have a new concept for all tx-related methods - finality. finality in the response is merged to master 2 weeks ago: add status field to response in RPC methods tx, EXPERIMENTAL_tx_status, broadcast_tx_commit, broadcast_tx_async #9556. finality field in the request means "I want at least this level of confidence". So, the stricter the level, the longer the user needs to wait.
I decided to use Final as a default finality value because it gives the strongest guarantee and does not change backward compatibility for any of the methods (though, the waiting time may be increased sometimes to achieve the strictest level of confidence).
Methods tx, EXPERIMENTAL_tx_status have finality as an additional optional field.
A new method appeared - broadcast_tx. It allows to send the tx, it also have the optional field finality.
broadcast_tx_async is equal to broadcast_tx with hardcoded finality None, I'll mark it as deprecated in the doc
broadcast_tx_commit is equal to broadcast_tx with hardcoded finality Final, I'll mark it as deprecated in the doc.

telezhnaya · 2023-10-16T17:27:53Z

core/primitives/src/views.rs

 pub enum TxExecutionStatus {
    /// Transaction is waiting to be included into the block
    None,
    /// Transaction is included into the block. The block may be not finalised yet
-    Inclusion,
+    Included,


This change might be dangerous, but we merged this enum only few weeks ago, and it's not used in production yet.
I realised that it's better if all the options would be adjectives, it's more intuitive to see "included" near "executed"

telezhnaya · 2023-10-17T15:43:16Z

chain/jsonrpc/src/lib.rs

-                        break Err(err);
+                        if finality == TxExecutionStatus::None {
+                            break Err(err);
+                        }


With the ability to provide finality to requests of methods tx, EXPERIMENTAL_tx_status, and Final as the default status value, the behavior changes in some cases. It may be considered as a breaking of backward compatibility.
In particular, I can imagine 2 situations:

When the tx was just sent to the network, we previously showed "Unknown transaction" error next ~3-6 seconds. With this change, we will wait these ~3-6 seconds and then return the existing tx.

When the tx never existed (let's say it's the tx from testnet, and the user tries to find in on mainnet), and the user provides any finality other than None (or leaves the field blanc and we take the default value), user will wait for 10 seconds and then see Timeout error. Previously, we showed "Unknown transaction" almost immediately.

My opinion here:
The first one seems more like an improvement to me. (@khorolets will be happy because I will accidentally improve the correctness of Read RPC with this change)
The second one seems rare enough. If someone has a bunch of random hashes and they need to find all the existing transactions, they may use finality None to speed up the process

telezhnaya · 2023-10-17T15:59:55Z

chain/jsonrpc/src/lib.rs

+    > {
+        self.broadcast_tx(RpcBroadcastTransactionRequest {
+            signed_transaction: request_data.signed_transaction,
+            finality: TxExecutionStatus::Final,


Here, I have to hardcode finality because it's the handler of the legacy method.
Digging into the code, I see that we never checked in broadcast_tx_commit if the tx was included into the canonical chain. Ofk, same applies to all the corresponding receipts.
At the same time, the doc says that broadcast_tx_commit gives strictest guarantees that we may give: https://docs.near.org/api/rpc/transactions#send-transaction-await
I think we have never seen any issues here because the situation of parallel chains is very rare, so the majority of the existing blocks were included into the canonical chain.
Moreover, if the receipts chain is long enough (3+ sequential receipts), we have a huge probability that the original transaction's block was finalized. broadcast_tx_commit waits for all the execution outcomes, so accidentally it waits for tx finalization as well.
The closest finality we defined to this behavior is Executed.

However, I think we should use Final here because broadcast_tx_commit was always promoted as the method with the strictest guarantees, so let's follow these expectations.

telezhnaya · 2023-10-17T16:05:45Z

@khorolets @firatNEAR I've added you to the reviewers because I want your opinion about the changes to the interface I've proposed.
You may totally ignore the code changes, it's enough just to read the comments I've left and argue with me :)

@nikurt sorry but you need to look at the code as well

khorolets

I didn't find anything to argue with. I've left a small comment, though.

While I agree with the changes and cleanups you did, it seems like you have refactored some of the methods changing the legacy behavior. Don't get me wrong, this is nice and all, but I think it'd be better to add all of these like EXPERIMENTAL_ while keeping the legacy, since we don't have a way to actually deprecate the stuff.

I don't insist, though.

chain/jsonrpc/src/lib.rs

nikurt

The code looks good.
Inclusion->Included is a good change.
Thank you for the comments, that makes reviewing easier :)

nikurt · 2023-10-18T09:16:43Z

chain/jsonrpc/src/lib.rs

@@ -312,6 +314,9 @@ impl JsonRpcHandler {
        Ok(match request.method.as_ref() {
            // Handlers ordered alphabetically
            "block" => process_method_call(request, |params| self.block(params)).await,
+            "broadcast_tx" => {


The word broadcast bothers me a bit.
It's not a broadcast (doesn't send a message to everyone), and it exposes an implementation detail.
In the documentation this method is described as "Send transaction", which I like more.

That's bikeshedding, and I don't suggest to rename existing methods, but maybe this new method can have a better naming.

I decided it's worth discussing
https://near.zulipchat.com/#narrow/stream/295302-general/topic/Method.20name.20for.20adding.20transaction.20to.20the.20chain/near/397287445

firatNEAR

I left a minor comment about 'EXPERIMENTAL_', other than that LGTM.

Allows to wait until the desired finality is reached

…hanges

telezhnaya

@nikurt please have a look

telezhnaya · 2023-10-23T20:05:55Z

chain/client/src/view_client.rs

+        // previous blocks may be not in the canonical chain
+        Ok(match self.chain.check_blocks_final_and_canonical(&headers) {
+            Err(_) => TxExecutionStatus::Executed,
+            Ok(_) => TxExecutionStatus::Final,


There was a bug in this code. For InclusionFinal, there was no check of block finality.
I've rewritten it fully. It's not written in a conventional Rust way, but, imho, now it's much easier to read.

telezhnaya · 2023-10-23T20:13:51Z

chain/jsonrpc/jsonrpc-tests/src/lib.rs

@@ -33,7 +33,7 @@ pub fn start_all_with_validity_period_and_no_epoch_sync(
    enable_doomslug: bool,
 ) -> (Addr<ViewClientActor>, tcp::ListenerAddr) {
    let actor_handles = setup_no_network_with_validity_period_and_no_epoch_sync(
-        vec!["test1".parse().unwrap(), "test2".parse().unwrap()],
+        vec!["test1".parse().unwrap()],


Before this change, we miss every second block -> we have no finalised blocks

As I'm not familiar with these tests I don't have an opinion whether 1 validator is always a good idea.
I assume it is, the fewer moving pieces, the simpler the tests.

telezhnaya · 2023-10-23T20:20:34Z

chain/jsonrpc/jsonrpc-tests/tests/rpc_transactions.rs

+                            .tx(RpcTransactionStatusRequest {
+                                transaction_info: TransactionInfo::TransactionId {
+                                    tx_hash,
+                                    sender_account_id: signer_account_id,


I've copy-pasted this logic from chain/jsonrpc/src/lib.rs
And added explicit naming

And with these changes, I'm curious
How will it work with meta tx?
And is sender_account_id naming correct here? (I've copy pasted this from the doc)

upd that comment should be here https://github.com/near/nearcore/pull/9644/files#diff-b0dd5d6f8f4d2a7b135860666232d02f02a60ee1dd9d7fd65ee6afe79550e98eR80
It's not about tests, it's about general implementation in file chain/jsonrpc-primitives/src/types/transactions.rs

In meta tx doc, we also have this sender/signer mess
https://nomicon.io/RuntimeSpec/Actions#delegate-actions

struct DelegateAction { /// Signer of the delegated actions pub sender_id: AccountId, ... }

Code has simply sender_id, receiver_id:

nearcore/core/primitives/src/action/delegate.rs

Line 21 in 3a56da2

pub struct DelegateAction {

Should be fine

telezhnaya · 2023-10-23T20:32:36Z

chain/jsonrpc-primitives/src/types/transactions.rs

+}
+
+#[derive(Clone, Debug, serde::Serialize, serde::Deserialize)]
+pub enum SignedTransaction {


FYI this +1 nesting enum is required to make serde's annotations magic work.
I've added functions below to avoid unpacking it each time

telezhnaya · 2023-10-23T20:35:15Z

chain/jsonrpc/jsonrpc-tests/tests/rpc_query.rs

@@ -454,7 +454,7 @@ fn test_validators_ordered() {
            .unwrap();
        assert_eq!(
            validators.into_iter().map(|v| v.take_account_id()).collect::<Vec<_>>(),
-            vec!["test1".parse().unwrap(), "test2".parse().unwrap()]
+            vec!["test1".parse().unwrap()]


I treat this test as "we have this method and it works at least somehow"
Previous 2 validators did not help us with checking if the order is correct (whatever it should be).

nikurt · 2023-10-24T08:44:17Z

chain/jsonrpc/jsonrpc-tests/src/lib.rs

@@ -33,7 +33,7 @@ pub fn start_all_with_validity_period_and_no_epoch_sync(
    enable_doomslug: bool,
 ) -> (Addr<ViewClientActor>, tcp::ListenerAddr) {
    let actor_handles = setup_no_network_with_validity_period_and_no_epoch_sync(
-        vec!["test1".parse().unwrap(), "test2".parse().unwrap()],
+        vec!["test1".parse().unwrap()],


As I'm not familiar with these tests I don't have an opinion whether 1 validator is always a good idea.
I assume it is, the fewer moving pieces, the simpler the tests.

nikurt · 2023-10-24T08:46:17Z

chain/jsonrpc/jsonrpc-tests/tests/rpc_transactions.rs

+                            .tx(RpcTransactionStatusRequest {
+                                transaction_info: TransactionInfo::TransactionId {
+                                    tx_hash,
+                                    sender_account_id: signer_account_id,


Code has simply sender_id, receiver_id:

nearcore/core/primitives/src/action/delegate.rs

Line 21 in 3a56da2

pub struct DelegateAction {

Should be fine

…, add method `broadcast_tx` (#9644) Fixes #6837 I'll need to update the [docs](https://github.com/near/docs) and [jsonrpc_client](https://github.com/near/near-jsonrpc-client-rs) (and maybe something else, I haven't already checked) Important updates: 1. We have a new concept for all tx-related methods - `finality`. `finality` in the response is merged to master 2 weeks ago: #9556. `finality` field in the request means "I want at least this level of confidence". So, the stricter the level, the longer the user needs to wait. 2. I decided to use `Final` as a default `finality` value because it gives the strongest guarantee and does not change backward compatibility for any of the methods (though, the waiting time may be increased sometimes to achieve the strictest level of confidence). 3. Methods `tx`, `EXPERIMENTAL_tx_status` have `finality` as an additional optional field. 4. A new method appeared - `broadcast_tx`. It allows to send the tx, it also have the optional field `finality`. 6. `broadcast_tx_async` is equal to `broadcast_tx` with hardcoded `finality None`, I'll mark it as deprecated in the doc 7. `broadcast_tx_commit` is equal to `broadcast_tx` with hardcoded `finality Final`, I'll mark it as deprecated in the doc.

near#9644 added a new RPC method for sending transactions, and added unused "wait_until" arguments to the existing ones. This change was actually not a no-op for the "broadcast_tx_commit" method, since now that method will wait until all transactions and descendant receipts are final, whereas the previous behavior was closer to the new "INCLUDED" "wait_until" parameter, and we used to just wait for the receipt outcomes to show up, but would return before they're executed and finalized. So fix it by making send_tx_and_wait() call the send_tx method to get something closer to the old behavior. Also, an unrelated problem with this test is that the "consensus" changes to the config were put in the wrong place, so that measure to decrease the flakiness of this test wasn't actually in place

#9644 added a new RPC method for sending transactions, and added unused "wait_until" arguments to the existing ones. This change was actually not a no-op for the "broadcast_tx_commit" method, since now that method will wait until all transactions and descendant receipts are final, whereas the previous behavior was closer to the new "INCLUDED" "wait_until" parameter, and we used to just wait for the receipt outcomes to show up, but would return before they're executed and finalized. So fix it by making send_tx_and_wait() call the send_tx method to get something closer to the old behavior. Also, an unrelated problem with this test is that the "consensus" change to the config were put in the wrong place, so that measure to decrease the flakiness of this test wasn't actually in place

bowenwang1996 · 2024-02-20T21:23:54Z

@telezhnaya question: from the PR description it seems that we broadcast_tx_commit now waits for a transaction is finalized before returning. I am pretty sure the behavior before was that it just wait for the transaction to finish, not necessarily finalized. What prompted the change in behavior? cc @gmilescu

telezhnaya · 2024-02-20T22:21:14Z

@bowenwang1996 we promoted this method as the one which gives 100% guarantees that the transaction was finished [in a way how people usually understand this, not in a Near-specific terms]. In reality, the transaction could be included and executed inside non-canonical chain which would be fully ignored later. We never explained this anywhere.
After the discussion with other engineers, I decided to change this behaviour and add some more guarantees.

Do we have any issues because of this change?

bowenwang1996 · 2024-02-26T15:43:17Z

@telezhnaya yes please revert this part of the change. The issue is that it will introduce a nontrivial amount of latency (3s in the good case, potentially much longer if blocks or chunks are missed), which would make a lot of UI that relies on real time confirmation (such as wallets) break. I wonder whether this is also related to the latency issue that the SRE team is investigating. cc @rtsainear

telezhnaya · 2024-02-26T16:12:25Z

@bowenwang1996 let's leave the check if the block with the initial transaction is finalised?
I want to set the guarantee to TxExecutionStatus::Executed

nearcore/core/primitives/src/views.rs

Lines 1706 to 1721 in 8ffa7f2

    
           pub enum TxExecutionStatus { 
        
               /// Transaction is waiting to be included into the block 
        
               None, 
        
               /// Transaction is included into the block. The block may be not finalised yet 
        
               Included, 
        
               /// Transaction is included into finalised block 
        
               IncludedFinal, 
        
               /// Transaction is included into finalised block + 
        
               /// All the transaction receipts finished their execution. 
        
               /// The corresponding blocks for each receipt may be not finalised yet 
        
               Executed, 
        
               /// Transaction is included into finalised block + 
        
               /// Execution of transaction receipts is finalised 
        
               #[default] 
        
               Final, 
        
           }

It sounds like a good balance between the speed (we no longer wait for the finalisation of blocks with all the receipts) and the guarantees (the block with the transaction is finalised, it means that sooner or later all the receipts will also be included into finalised blocks).

bowenwang1996 · 2024-02-26T23:32:03Z

@telezhnaya in general I am not against discussing what the default behavior of this method should be and making changes to it. However, given the vast impact it has on millions of users that are using NEAR today, I think we should be very careful with rolling it out. As far as I know, all the major apps (HOT, Kaiching, Sweat) rely on the endpoint being fast. So I suggest we revert the change to the default behavior and start a separate discussion as to what it should be changed to and make the community aware of the change before hand, rather than doing it unilaterally.

telezhnaya · 2024-02-27T12:28:04Z

OK, I'll return the logic of broadcast_tx_commit back as it was before the changes.
FYI, I'll do this not by reverting, but by the new commit, so that we can still have the other changes (e.g. adding new method send_tx).

telezhnaya force-pushed the add_tx_request_parameter branch from bb7062c to 107e4e5 Compare October 5, 2023 13:36

telezhnaya force-pushed the add_tx_request_parameter branch 4 times, most recently from 3ee947f to e577372 Compare October 16, 2023 17:17

telezhnaya changed the title ~~wip~~ jsonrpc: add finality to tx requests (tx, EXPERIMENTAL_tx_status), add method broadcast_tx Oct 16, 2023

telezhnaya commented Oct 16, 2023

View reviewed changes

telezhnaya force-pushed the add_tx_request_parameter branch 3 times, most recently from d9e4f26 to 1b113c0 Compare October 17, 2023 14:04

telezhnaya commented Oct 17, 2023

View reviewed changes

telezhnaya force-pushed the add_tx_request_parameter branch 2 times, most recently from 0febd67 to be36a7a Compare October 17, 2023 15:47

telezhnaya commented Oct 17, 2023

View reviewed changes

telezhnaya marked this pull request as ready for review October 17, 2023 16:00

telezhnaya requested a review from a team as a code owner October 17, 2023 16:00

telezhnaya requested review from nikurt, khorolets and firatNEAR October 17, 2023 16:00

khorolets previously approved these changes Oct 18, 2023

View reviewed changes

chain/jsonrpc/src/lib.rs Outdated Show resolved Hide resolved

nikurt previously approved these changes Oct 18, 2023

View reviewed changes

firatNEAR previously approved these changes Oct 18, 2023

View reviewed changes

telezhnaya dismissed stale reviews from firatNEAR, nikurt, and khorolets via 58b96be October 18, 2023 18:48

telezhnaya force-pushed the add_tx_request_parameter branch 2 times, most recently from 2a616a9 to 3396a9c Compare October 18, 2023 19:12

nikurt previously approved these changes Oct 19, 2023

View reviewed changes

telezhnaya dismissed nikurt’s stale review via 80b26f3 October 19, 2023 12:49

telezhnaya enabled auto-merge October 19, 2023 18:19

telezhnaya dismissed nikurt’s stale review via c7b8e6d October 20, 2023 23:46

telezhnaya force-pushed the add_tx_request_parameter branch from 7fcf9e0 to 25496ce Compare October 21, 2023 00:15

telezhnaya added 2 commits October 23, 2023 21:01

jsonrpc: add finality to tx requests

9b8d3c3

Allows to wait until the desired finality is reached

jsonrpc: rename finality -> wait_until, rollback broadcast_tx_async c…

28f4f7f

…hanges

telezhnaya force-pushed the add_tx_request_parameter branch from 181729c to ef31e35 Compare October 23, 2023 20:02

jsonrpc: fix parsing signed tx object

2f63fde

telezhnaya force-pushed the add_tx_request_parameter branch from ef31e35 to 2f63fde Compare October 23, 2023 20:30

telezhnaya commented Oct 23, 2023

View reviewed changes

telezhnaya disabled auto-merge October 23, 2023 21:22

nikurt approved these changes Oct 24, 2023

View reviewed changes

telezhnaya added this pull request to the merge queue Oct 24, 2023

Merged via the queue into near:master with commit fa1cfa1 Oct 24, 2023
14 checks passed

telezhnaya deleted the add_tx_request_parameter branch October 24, 2023 09:17

marcelo-gonzalez mentioned this pull request Nov 9, 2023

fix(pytests): fix rpc_finality.py #10148

Merged

telezhnaya mentioned this pull request Feb 27, 2024

1.37 release timeline #10404

Closed

gtsonevv mentioned this pull request Feb 28, 2024

feat: add support for nearcore 1.37 changes LimeChain/near-api-js#12

Closed

5 tasks

telezhnaya mentioned this pull request Feb 28, 2024

revert broadcast_tx_commit checks #10655

Merged

bowenwang1996 mentioned this pull request Mar 4, 2024

Introduce receipt depth in broadcast_tx endpoint #10697

Open

gtsonevv mentioned this pull request Mar 12, 2024

Nearcore 1 37 changes near/near-api-js#1317

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

jsonrpc: add finality to tx requests (`tx`, `EXPERIMENTAL_tx_status`), add method `broadcast_tx` #9644

jsonrpc: add finality to tx requests (`tx`, `EXPERIMENTAL_tx_status`), add method `broadcast_tx` #9644

telezhnaya commented Oct 5, 2023 •

edited

Loading

telezhnaya Oct 16, 2023

telezhnaya Oct 17, 2023

telezhnaya Oct 17, 2023 •

edited

Loading

telezhnaya commented Oct 17, 2023 •

edited

Loading

khorolets left a comment

nikurt left a comment

nikurt Oct 18, 2023 •

edited

Loading

telezhnaya Oct 18, 2023

firatNEAR left a comment

telezhnaya left a comment

telezhnaya Oct 23, 2023

telezhnaya Oct 23, 2023

nikurt Oct 24, 2023

telezhnaya Oct 23, 2023 •

edited

Loading

telezhnaya Oct 24, 2023

nikurt Oct 24, 2023

telezhnaya Oct 23, 2023

telezhnaya Oct 23, 2023

nikurt Oct 24, 2023

nikurt Oct 24, 2023

bowenwang1996 commented Feb 20, 2024

telezhnaya commented Feb 20, 2024

bowenwang1996 commented Feb 26, 2024

telezhnaya commented Feb 26, 2024

bowenwang1996 commented Feb 26, 2024

telezhnaya commented Feb 27, 2024

jsonrpc: add finality to tx requests (tx, EXPERIMENTAL_tx_status), add method broadcast_tx #9644

jsonrpc: add finality to tx requests (tx, EXPERIMENTAL_tx_status), add method broadcast_tx #9644

Conversation

telezhnaya commented Oct 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

telezhnaya Oct 17, 2023 • edited Loading

Choose a reason for hiding this comment

telezhnaya commented Oct 17, 2023 • edited Loading

khorolets left a comment

Choose a reason for hiding this comment

nikurt left a comment

Choose a reason for hiding this comment

nikurt Oct 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

firatNEAR left a comment

Choose a reason for hiding this comment

telezhnaya left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

telezhnaya Oct 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bowenwang1996 commented Feb 20, 2024

telezhnaya commented Feb 20, 2024

bowenwang1996 commented Feb 26, 2024

telezhnaya commented Feb 26, 2024

bowenwang1996 commented Feb 26, 2024

telezhnaya commented Feb 27, 2024

jsonrpc: add finality to tx requests (`tx`, `EXPERIMENTAL_tx_status`), add method `broadcast_tx` #9644

jsonrpc: add finality to tx requests (`tx`, `EXPERIMENTAL_tx_status`), add method `broadcast_tx` #9644

telezhnaya commented Oct 5, 2023 •

edited

Loading

telezhnaya Oct 17, 2023 •

edited

Loading

telezhnaya commented Oct 17, 2023 •

edited

Loading

nikurt Oct 18, 2023 •

edited

Loading

telezhnaya Oct 23, 2023 •

edited

Loading