Issue with access key nonce

Bowen · February 24, 2021, 6:52pm

Background

Currently we have the following issue with access key nonce: if someone deletes a key and adds the same key back with nonce 0, then old transactions signed with that key can potentially be submitted and processed again, which may cause trouble for the end users.

Solutions

There are in general two different categories of solutions, representing different views of the problem.

Address the issue on the tooling level

The reasoning here is that users are not supposed to delete a key and add the same key back. This is a practice that can shoot themselves in the foot and must be avoided. Consequently, to prevent users from falling to this trap, client sdks should take care of the issue so that developers don’t have to worry about it. For example, we could always use current_block_height * 1e6 as a nonce for newly created access keys, or simply use block timestamp. In the context of near-api-js, this would mean changing the following two functions to take nonce as an argument

near-api-js/transaction.ts at 5265f065ec6f7148928711e6663e51a4ca51738d · near/near-api-js · GitHub
near-api-js/transaction.ts at 5265f065ec6f7148928711e6663e51a4ca51738d · near/near-api-js · GitHub
Correspondingly, we should add this to our docs on access keys so that developers working on devtools are aware of the consequences.

However, one issue with this approach, besides being less reliable, is that this does not prevent duplicate transactions from being included onchain, which, while on its own is fine, causes trouble for rpc and indexer since transaction hash can no longer uniquely identify a transaction.

Address the issue on the protocol level

There are multiple ways to address this issue completely on the protocol level. One way that seems to work well is to require the access key nonce to be h * 1e6 where h is the block in which the add key transaction is included and for every transaction signed with the access key afterwards, they must have a nonce no larger than h * 1e6, where h is the block in which the transaction is included.

The disadvantage with this approach, besides needing to modify the protocol, is that we impose a range restriction on the access key nonces and as a result, it is not feasible if someone wants to create a key locally, sign some transactions with the key and then submit the add key transaction together with the transactions signed by the key. Note that we are not aware of anyone (including custody provider) who is doing this currently, so it might not be an actual issue.

mikedotexe · February 24, 2021, 7:10pm

I didn’t realize until this conversation that the tools were hardcoding the 0 into the nonce for an access key’s creation. To those who are curious, here’s where it’s at in near-api-js, lines highlighted and pinned to the latest commit at the time of this post:

github.com

near/near-api-js/blob/5265f065ec6f7148928711e6663e51a4ca51738d/src/transaction.ts#L27-L33


export function fullAccessKey(): AccessKey {
    return new AccessKey({ nonce: 0, permission: new AccessKeyPermission({fullAccess: new FullAccessPermission({})}) });
}
export function functionCallAccessKey(receiverId: string, methodNames: String[], allowance?: BN): AccessKey {
    return new AccessKey({ nonce: 0, permission: new AccessKeyPermission({functionCall: new FunctionCallPermission({receiverId, allowance, methodNames})})});
}

frol · February 24, 2021, 9:29pm

Some data points from Indexer infrastructure based on @alexatnear questions:

Max nonce that is currently observed on mainnet is 966780, and on testnet it is 247105
On mainnet there is no single ADD_KEY receipt action that uses non-zero nonce
On testnet there are 4 (four) receipts that set a non-zero nonce (the range is from 1 to 74)

vlad · February 25, 2021, 12:50am

I suspect on testnet these come from @mattlockyer testing workaround for the issue

mattlockyer · February 25, 2021, 1:00am

Unclear based on what @frol posted if that was from me.

frol · February 25, 2021, 6:39pm

Nope, those are not your transactions; those are related to my near-cli-rs testing. Did you test that 2FA flow fix on testnet or mainnet? I wonder why I don’t see them. In fact, what I think is 2FA flow still adds keys with zero nonce: NEAR Explorer | Transaction (it is a recent transaction)

frol · February 25, 2021, 7:11pm

Addressing issue on the protocol level is hard, but feasible. Addressing any issue on the tooling side is impossible since tools will evolve and new ones will get created.

In order to “fix” it on tooling side, we will need to BREAK all the tooling requiring to specify the block hash/height whenever you want to know the transaction status. You may argue that the tooling is already broken, but I would say that only the tooling that absolutely requires to handle every single transaction is broken. Even Explorer is not that sensitive (well, I know it is bad that we cannot display the transaction properly, but do you really care about that one glitch transaction right now?).

I believe that if you ask partners if they would prefer (1) a breaking API change (which will require extra data to pass, so requiring them to store block hash/height along with the transaction itself) OR (2) a fix on protocol level with potentially a hard-fork, most of them will chose the latter.

Using Explorer as a case study, as a user I want to paste the transaction hash and be done with that. If we have two transactions to display, we may show two search results in the drop-down for selection, but overall, I believe it would be quite confusing.

Is there a blockchain which tolerates transaction hash collisions on the protocol level? Should we define “transaction id” as something like block_hash:account_id:transaction_hash?

Bowen · March 9, 2021, 7:47pm

After discussing with @frol @alexatnear @evgenykuzyakov we decide to move forward with a protocol level fix, which is submitted here.

nearmax · March 15, 2021, 11:40pm

I think this should’ve been NEP.

Bowen · March 16, 2021, 12:09am

Yes, but we also want to get this change in quickly to avoid further issues (duplicate transactions) that we have to handle on the side of indexer.

mikedotexe · May 7, 2021, 5:11pm

So it feels like we can remove the explicit setting of nonce: 0 now? This might confuse future developers. Wondering if the protocol is expecting this key or not, or if it’s backwards compatible @Bowen

Bowen · May 7, 2021, 5:14pm

Yes we probably should do that to make it less confusing. The field still exists though.