-
Notifications
You must be signed in to change notification settings - Fork 161
Draft NEP for pending transaction queue. #611
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
base: master
Are you sure you want to change the base?
Conversation
birchmd
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for bringing this up! It is good to think about how to prevent possible DoS attacks under SPICE.
neps/nep-0611.md
Outdated
| #### Access Key Parallelism Restriction | ||
| We now restrict the ability to send multiple parallel pending transactions with Access Keys. | ||
|
|
||
| Specifically, for any given account $A$ with any number of access keys, the total number of access key transactions in the pending transaction queue whose sender is $A$ cannot exceed $P_{\mathrm {max}}$, a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this enough to prevent the attack described above? Instead of having one account they send many transactions to, it could be many accounts they send only a few transactions to each.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea is that the amount of block space the attacker consumes vs amount of gas the attacker has to pay, i.e. the "waste amplification factor", is unbounded in the attack mentioned in the NEP, but only O(1) with a maximum parallelism. It's then still possible to attack, but the attack would cost a proportional amount of gas - just cheaper than it is today - which isn't a very great incentive to mount the attack (given that the attack is only a DoS).
See near/NEPs#611 This PR adds the GasKey trie key. In subsequent PRs we will introduce a new transaction type to accept gas key transactions, and to implement the gas key actions.
See near/NEPs#611 This PR adds the GasKey trie key. In subsequent PRs we will introduce a new transaction type to accept gas key transactions, and to implement the gas key actions.
|
Hi @robin-near (or anyone who is interested in championing this NEP forward) – we are cleaning the NEP backlog and noticed that this PR still has some incomplete sections: particularly, it has a "TODO: Other things". Therefore, we are labeling this PR as "Needs author revision." Please ping the @near/nep-moderators once you are ready for us to review it. We typically close NEPs that are inactive for more than two months, so please let us know if you need more time. |
|
Because there has been no activity from the author for more than 3 months, I'm marking the proposal as RETRACTED. Please feel free to open it again if you'd like to continue working on it. |
|
As an SME, I have reviewed the proposal. Kudos to the team behind it, it is well designed thought through. Special thanks to @darioush for finalizing the NEP and being very responsive to any questions and suggestions I had. Technical summaryThe NEP builds the basis for a new concept, the pending transaction queue (PTQ). This queue is for transactions that are included for execution but have not been applied to the latest known blockchain state, yet. The PTQ allows decoupling consensus on new chunks from transaction execution. Transactions to be included in a new chunk are verified against the state of an older block height, plus all pending transactions. The challenge with this approach is that a transaction may look valid when it enters the PTQ but is no longer valid once it gets executed. This is especially problematic if the signer no longer has the balance to pay for the gas cost it owes for handling the transaction, or if the signing key got removed and it is no longer known whether it was a valid signature in the first place. To address these concerns, a new type of access key is introduced, called gas key. Gas keys have a balance reserved for gas that cannot be removed without it being visible in the PTQ. Therefore, it is possible to verify ahead of time that a transactions signed with a gas key will have the balance to cover the gas cost once it executes. For transactions signed by normal access keys, such a verification is not possible. Each account will be limited to a small number (4) of transactions per account in the PTQ to reduce the number of transactions that may go through the PTQ without being charged in the end. RecommendationI recommend accepting this proposal to move SPICE development forward. See benefits and concerns below for the arguments in both directions. Benefits
Concerns
|
Co-authored-by: Matej Pavlovic <[email protected]>
neps/nep-0611.md
Outdated
|
|
||
| `DeleteAccount` action will fail if the account has any associated gas keys. | ||
|
|
||
| `DeleteKey` is not allowed to delete gas keys. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to not introduce a separate DeleteGasKey action and instead modify the DeleteKey action and define it as follows?
"If the key is an access key, <whatever the current definition of DeleteKey is>. If the key is a gas key, "
I imagine the separate action for deleting gas keys might be an artifact from the times where we considered gas keys and access keys to have different name spaces. Now that gas keys share the namespace with access keys, the two separate actions don't seem necessary. Moreover, I'd personally argue that a single DeleteKey is much more intuitive from a UX perspective.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Realizing that another difference between these two actions is nonce deletion, I see that the proposed simplification might not work. I guess this UX hiccup is something we'll have to live with.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The other (main) reason to keep it separate is that contracts can currently request DeleteKey via the promise_batch_action_delete_key host call.
^ This was not correct.
Actually, we can go with this approach based on this feedback and feedback from others.
The downside is we cannot charge gas for deletion accurately, but this seems not a practical issue.
We can use compute costs to prevent too much work in one chunk, and we could collect the gas ahead of time during addition if we really care about it.
As the design of the gas keys has diverged from the original, I think a revert can set a good baseline to review the new code carefully instead of trying to make a "migration" from the implementation to the new one (#14788) The up to date design can be found here: near/NEPs#611
As the design of the gas keys has diverged from the original, I think a revert can set a good baseline to review the new code carefully instead of trying to make a "migration" from the implementation to the new one (#14788) The up to date design can be found here: near/NEPs#611
…4891) Updates add/delete key actions to handle gas keys per near/NEPs#611 Note this also moves the code for add/delete keys to a new `access_keys` module. Additional changes: - Added a new `QueryRequest::ViewGasKeyNonces` variant and its handling in `NightshadeRuntime`, allowing clients to query the nonces associated with gas keys for a given `account_id` and `public_key`. The response is provided as a new `QueryResponseKind::GasKeyNonces` variant. - Implemented the `view_gas_key_nonces` method in the runtime adapter, which retrieves gas key nonces from the state trie. - Introduced new error variants for gas key operations in `ActionsValidationError`: `GasKeyTooManyNoncesRequested`, `AddGasKeyWithNonZeroBalance`, and `GasKeyFunctionCallAllowanceNotAllowed` - Removed the obsolete `KeyPermissionInvalid` error variant from both `ActionsValidationError` and `ActionErrorKind`, a
matejpavlovic
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
All the comments I added are just nit picking.
| * Check that if the permission is a `FunctionCallPermission`, the allowance is `None` (unlimited | ||
| allowance). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the case for access keys as well? If yes, I guess we require this for consistency - all good. If not, is there a specific reason we don't allow limiting the allowance for keys with FunctionCallPermission?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ordinary access keys can use the allowances.
One reason is to avoid having to track them in the pending transaction queue (similar to balances).
I don't see a fundamental reason, so I guess we could add it later on if it was a request or requirement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean that we'd need to track the allowance changes? Would the problematic case be wrongly accepting a transaction that exceeds the allowance because the allowance has only recently been decreased by some transaction in the pending queue? If that's the idea, then it makes sense to me - feel free to resolve this comment.
| To compute the pending transaction queues (one for each tracked shard) for a new block, | ||
|
|
||
| * Start from the queue from the previous block; | ||
| * Subtract the transactions that are included in each newly certified block; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reader needs to know how SPICE works to understand this. Maybe we should mention that and repeat the remark that if this NEP is implemented on the current system, the pending transaction queue will always be empty anyway.
Co-authored-by: Matej Pavlovic <[email protected]>
|
Hi everyone! seems like SME @jakmeier and @matejpavlovic have completed their technical reviews. Based on their comments above, it seems like this proposal is ready for the working group members for review. @near/wg-protocol – Can you please fully read this NEP and comment in the thread if you are leaning towards approving or rejecting it? Please make sure to include your rationale and any feedback that you have for the author. Once we get 2/3 voting indications, we can start scheduling a public call for the author to present the NEP and for the working group members to formalize the voting. |
birchmd
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a protocol working group member, I lean towards approving this proposal.
I have left some minor comments, but overall I think this proposal is well-motivated and technically sound. Rolling out SPICE-related changes as small, self-contained and backwards-compatible pieces will ease the transition for all participants in the Near ecosystem.
neps/nep-0611.md
Outdated
|
|
||
| #### Modifications to existing actions | ||
|
|
||
| `AddKey` action will fail if the public key is already registered as a gas key (unless adding a gas key with that public key). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the statement in paratheses here meant to imply that the action to add a gas key is idempotent? I think it makes sense to fail a duplicate add transaction instead of having it silently do nothing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The duplicate add transaction is intended to fail. The writing here is confusing, I updated the paragraph.
neps/nep-0611.md
Outdated
| * To apply this action, all relevant trie nodes are deleted, | ||
| * Decreases storage usage of associated account, | ||
| * The remaining balance left in the key is **burned**. This prevents an attack where an adversary floods the chain with transactions, then deletes the key to reclaim the balance before the transactions execute. | ||
| * For user's benefit we may consider failing this action if the balance exceeds a certain threshold. The implementation may include a `force` flag that overrides this check. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think having this threshold is a good idea and should be included officially in the NEP rather than being left as an ambiguous maybe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call, we will discuss in the SPICE implementation group on Monday and update the NEP with a suitable value.
Please let me know if you have an opinion about the force flag as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We suggested 1 NEAR as this threshold.
Additionally the force option is a leftover from a previous revision where we introduced separate actions to delete gas keys. To keep the same action, we can't add such option, so I removed its mention here.
neps/nep-0611.md
Outdated
| ```rust | ||
| enum TransactionNonce { | ||
| Nonce { nonce: Nonce }, | ||
| GasKeyNonce { nonce: Nonce, nonce_index: u32 } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: if we the limit on parallel nonces will be 1024 then we could use u16 here for the nonce index instead of u32 just to save a few bytes of space in the serialized form.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, with u16 we can still increase the max to ~65k nonces.
I will also discuss this on Monday with the implementation group.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to u16.
neps/nep-0611.md
Outdated
|
|
||
| Transaction processing is extended with a new failure case: `InvalidTxError::NotEnoughBalanceForDeposit`. In case the account programmatically sends away its balance we need to be able to charge the user a fee, otherwise we will have the spam issue this NEP intends to prevent. With honest chunk producers, this case only happens when using a gas key. | ||
|
|
||
| *Note*: As of the current date, `nearcore` ignores failed transactions in processing. After `ProtocolFeature::InvalidTxGenerateOutcomes`, invalid transactions impact the outcome but not the state. Therefore, this NEP also introduces the change that failed transactions may update the state (to charge for gas). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do I understand correctly that this statement means the gas key balance will be reduced even in the case of a failed transfer due to insufficient balance? I think that does make sense for DoS prevention, I am just double checking because it's not fully clear to me in the text as written.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's correct. In this scenario, the gas key balance is reduced to charge for gas even though the transaction fails.
I'll clarify this in the text.
neps/nep-0611.md
Outdated
| This constraint should be good enough to cover cases of adding, funding, removing, or withdrawing from gas | ||
| keys as well. For adding and funding, we do need the execution to catch up before the newly available | ||
| balance can be used for pending transactions, which is suboptimal but correct. | ||
| Note that contract execution cannot create receipts that withdraw from gas keys; only gas key |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is because we are intentionally not updating the promise-creating host functions in the VM to include the new action? I'm not sure where it is appropriate to do so, but it would be good to document that these actions are intentionally excludes as opposed to missed by an oversight so that it is not accidentally added in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's correct. Updating to capture this comment.
In the near future of the Near blockchain, we foresee that via the SPICE project, transaction and receipt execution will become decoupled from the blockchain itself; they will no longer run in lockstep. Instead, transactions will be included in the blocks first, and then execution will follow later.
This inherently introduces a problem that we must accept transactions before we know whether they are valid. Today, when a chunk producer produces a chunk containing a transaction, it can verify using the current shard state that the transaction has a valid signature, has enough balance, and a valid nonce. But as execution becomes asynchronous, we no longer have the current shard state to verify the
transactions against.
This NEP proposes a mechanism called the Pending Transaction Queue to solve this problem.