-
Notifications
You must be signed in to change notification settings - Fork 955
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
Fee specs #889
Fee specs #889
Conversation
|
||
Tendermint doesn't provide more than the `BlockSize.MaxGas` parameter, leaving the validation step to the application (see [tendermint spec](https://github.com/tendermint/tendermint/blob/29e5fbcc648510e4763bd0af0b461aed92c21f30/spec/core/data_structures.md#consensusparams) and [issue](https://github.com/tendermint/tendermint/issues/2310)): therefore, instead of using the Tendermint provided param, Namada introduces a `MaxBlockGas` protocol parameter. | ||
This limit is checked during block validation, in `process_proposal`: if the block exceeds the maximum amount of gas allowed, the validators will still accept the block and discard only the excess transactions. |
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.
To my understanding, Tendermint validators prevote for nil
when a block exceeds the size limit (which is a related topic). In here I propose instead to use the same logic we apply to invalid txs, discarding only the invalid ones and still accept the block (at the moment we reject the entire block only when the order of the decrypted txs doesn't match the order of the corresponding wrappers committed in the previous block).
We may think about rejecting the entire block for certain cases (in generale for those in which the proposer can check at block construction time whether a tx is valid or not) or for some sort of punishment in the form of slashing
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.
What is the benefit of accepting the block anyways? Is there any case where a correct proposer would accidentally create an invalid 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 only benefit is that we don't need to go through another Tendermint round for that specific block height, so we have an higher TPS in these cases.
A block proposer is able to determine whether a transaction is valid or not in most cases, expect for the inner tx wasm code (like trying to perform a transfer with not enough funds). At the moment, if not enough valid txs are available to fill the block, the proposer could place some invalid txs in there since he doesn't have a disincentive to do so. These invalid txs would be correctly rejected by the validators and the proposer could not redeem fees for these but there's still a small damage being done: since these invalid txs have been inserted in a block they will be removed from mempool once the block gets committed, forcing the submitters to resend them.
So I guess we can reject blocks containing invalid txs (for which validity can be checked at process_proposal
time)
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 the spec! Left some comments, and a general one: no unshielded wrapper transaction fee payments is a pretty serious drawback 😦 - I don't think we would need a full Transfer
transaction, but could we special-case a very simple MASP transaction part involving one unshielding action, perhaps?
The token whitelist consists of a list of $(T, GP_{min})$ pairs, where $T$ is a token identifier and $GP_{min}$ is the minimum (base) price per unit gas which must be paid by a transaction paying fees using that asset. This whitelist can be updated with a standard governance proposal. All fees collected are paid directly to the block proposer (incentive-compatible, so that side payments are no more profitable). | ||
|
||
Fees are distributed among the delegators with the mechanism explained in the [POS](./proof-of-stake/reward-distribution.md) specs. |
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.
Fees are distributed among the delegators with the mechanism explained in the [POS](./proof-of-stake/reward-distribution.md) specs. | |
Fees are distributed among the delegators with the mechanism explained in the [proof-of-stake reward distribution specs](./proof-of-stake/reward-distribution.md). |
Fees are distributed among the delegators with the mechanism explained in the [POS](./proof-of-stake/reward-distribution.md) specs. | ||
|
||
Fees are only meant for `InnerTx` transactions: `WrapperTx`s are not subject to them. |
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.
not sure I follow this - don't the fees need to be included in the outermost transaction? we have to check fees before accepting any transaction. also this seems to contradict the below
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 this is bad phrasing, what I meant is that even if fees are specified in the outer transactions they are relative to the inner one, meaning that the outer per se is not subject to fees. I'll rephrase this
The `WrapperTx` struct holds all the data necessary for the payment of fees in the form of the types: `Fee`, `GasLimit` and the `PublicKey` used to derive the address of the fee payer which coincides with the signer of the wrapper transaction itself. | ||
|
||
Since fees have a purpose in allocating scarce block resources (space and gas limit) they have to be paid upfront, as soon as the transaction is deemed valid and accepted into a block (refer to [replay protection](../base-ledger/replay-protection.md) specs for more details on transactions' validity). Moreover, for the same reasons, the fee payer will pay for the entire `GasLimit` allocated and not the actual gas consumed for the transaction: this will incentivize fee payers to stick to a reasonable gas limit for their transactions allowing for the inclusion of more transactions into a block. Since the gas used by a transaction leaks a bit of information about the transaction itself: a submitter may want to obfuscate this value a bit by increasing the gas limit of the wrapper transaction but he will be charged for this (refer to section 2.1.3 of the Ferveo [documentation](https://eprint.iacr.org/2022/898.pdf)). |
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.
Since fees have a purpose in allocating scarce block resources (space and gas limit) they have to be paid upfront, as soon as the transaction is deemed valid and accepted into a block (refer to [replay protection](../base-ledger/replay-protection.md) specs for more details on transactions' validity). Moreover, for the same reasons, the fee payer will pay for the entire `GasLimit` allocated and not the actual gas consumed for the transaction: this will incentivize fee payers to stick to a reasonable gas limit for their transactions allowing for the inclusion of more transactions into a block. Since the gas used by a transaction leaks a bit of information about the transaction itself: a submitter may want to obfuscate this value a bit by increasing the gas limit of the wrapper transaction but he will be charged for this (refer to section 2.1.3 of the Ferveo [documentation](https://eprint.iacr.org/2022/898.pdf)). | |
Since fees have a purpose in allocating scarce block resources (space and gas limit) they have to be paid upfront, as soon as the transaction is deemed valid and accepted into a block (refer to [replay protection](../base-ledger/replay-protection.md) specs for more details on transactions' validity). Moreover, for the same reasons, the fee payer will pay for the entire `GasLimit` allocated and not the actual gas consumed for the transaction: this will incentivize fee payers to stick to a reasonable gas limit for their transactions allowing for the inclusion of more transactions into a block. Since the gas used by a transaction leaks a bit of information about the transaction itself: a submitter may want to obfuscate this value a bit by increasing the gas limit of the wrapper transaction but they will be charged for this (refer to section 2.1.3 of the Ferveo [documentation](https://eprint.iacr.org/2022/898.pdf)). |
(fee payer may be a person or another sort of entity, we really don't know)
- Moving insufficient funds would incentivize the block proposer to include transactions for which fees cannot be paid. Since the block proposer knows the balances of the involved addresses at block creation time and given the strategy of placing wrapper txs first, this would constitute malicious behavior by the proposer | ||
|
||
By discarding the transaction without paying fees instead we avoid these pitfalls. This logic implies that the strategy of placing wrapper transactions before any decrypted tx in the block will be reinforced. It might look like a contradiction to what was said before, a wrapper transaction included in a block will not pay fees for the resources it acquired, but this is not true: |
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.
including a transaction which cannot pay fees should be considered an invalid block proposal
The signer of the wrapper transaction defines the token in which fees must be paid among those available in the token whitelist. At the same time, he also sets the amount which must meet the minimum price per gas unit for that token $GP_{min}$ (also defined in the whitelist). The difference between the minimum and the actual value set by the submitter represents the incentive for the block proposer to prefer the inclusion of this transaction over other ones. | ||
|
||
The block proposer can check the validity of these two parameters while constructing the block. These validity checks are also replicated in `process_proposal` and `mempool_check`. In case a transaction with invalid parameters ended up in a block, it would be discarded without paying any fee (as already explained earlier in this document). The block proposer and the `process_proposal` function should also cache the available funds of every fee-paying address involved in the block: this is because a signer might submit more than one transaction per block and the check on funds should take into consideration the updated value of the unshielded balance. |
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.
correct proposers should never create blocks with invalid transactions, and such block proposals should be rejected, if possible
## Gas accounting | ||
|
||
We provide a mapping between all the whitelisted transactions and VPs to their cost in gas units. Being the cost hardcoded, it is guaranteed that the same transaction will always require the same amount of gas: since the price per gas is controlled via governance, though, the price for the same transaction may vary in time. A transaction is also charged with the gas required by the validity predicates that it triggers. |
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.
what does "Being the cost hardcoded" mean? maybe "As the cost is constant, ..."
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.
here we need to define how exactly gas is measured, it's not clear to me from this part
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.
Correct, that part is missing. For this first implementation we could go with something as basic as the wasm size for a specific transaction/vp. At a later time we can come up with a better system taking into account the specific machine instructions executed
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.
alternatively, I think we could get some average run time for each whitelisted tx and use that until we have more fine-grained gas metering per ops
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, the run time looks like a better proxy for gas cost
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.
if runtime is not particularly input dependent then average runtime is OK but if it is input dependent then I'm not sure that would suffice
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 can expect the runtime to depend on the specific input, so yes this solution would just be an approximation (not necessarily close to real). This is just temporary while we develop a proper gas meter able to track the actual execution cost at runtime
|
||
Tendermint doesn't provide more than the `BlockSize.MaxGas` parameter, leaving the validation step to the application (see [tendermint spec](https://github.com/tendermint/tendermint/blob/29e5fbcc648510e4763bd0af0b461aed92c21f30/spec/core/data_structures.md#consensusparams) and [issue](https://github.com/tendermint/tendermint/issues/2310)): therefore, instead of using the Tendermint provided param, Namada introduces a `MaxBlockGas` protocol parameter. | ||
This limit is checked during block validation, in `process_proposal`: if the block exceeds the maximum amount of gas allowed, the validators will still accept the block and discard only the excess transactions. |
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.
What is the benefit of accepting the block anyways? Is there any case where a correct proposer would accidentally create an invalid block?
When executing the VPs the procedure is the same and, again, since we know ahead of time the gas required by each VP we can immediately terminate the execution if it overshoots the limit. | ||
|
||
In any case, if the gas limit is exceeded, the transaction is considered invalid and all the modifications applied to the WAL get discarded. This doesn't affect the other transactions which can be executed normally (see the following section). |
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 would just explicitly add here that the gas used for the tx that exceed its gas limit is still added to the used gas in the 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.
Actually, since the block gas limit has to be compared against the the GasLimit
declared in the WrapperTx
we don't need to monitor the actual gas usage of an InnerTx
against the block gas limit but only against the declared gas limit of the corresponding wrapper. So the gas meter could just check this constraint and avoid checking for the block gas limit since that is indirectly checked by the combination of these:
- checking in
process_proposal
that the cumulatedGasLimit
of alle the wrappers does not exceed the block gas limit - preventing every inner from exceeding the gas limit allocated by its wrapper
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.
mostly LGTM, just one nit
### Unshielding | ||
|
||
To prevent a possible locked-out problem in which a user doesn't have enough |
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.
and to provide improved privacy (that's the main goal)
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! A few comments/questions.
no more profitable). | ||
|
||
Fees are distributed among the delegators with the mechanism explained in the |
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.
No, they aren't - fees go only to the proposer. Proof-of-stake rewards are distributed to the delegators.
(otherwise it's profitable for the proposer to accept side payments instead)
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.
ah as we have a GP_{min}
we could actually pay gas * GP_min
to everyone if we wanted, and the difference to the proposer, that would also be incentive-compatible
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 think that this would be easy to do? it is slightly preferable if possible
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.
It is technically feasible but very inefficient. I think it would be better to keep it as is for the moment and then implement the distribution once #1125 has been completed
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.
wait why would this be inefficient?
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.
Because at the moment we don't have an easy way to get all the delegators for a specific validator. To do so we need to iterate over all the bonds deltas
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.
Ah, but staking fee distribution should be able to accomplish this (I don't suggest iterating). We can save this for a future upgrade, though.
block validation process. This is because a tx submitter could be side-paying | ||
the block proposer for tx inclusion which would prevent the correct distribution | ||
of fees among validators. The fair distribution of fees is enforced by the 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.
of fees among validators. The fair distribution of fees is enforced by the block | |
of fees among validators. The fair distribution of fees is enforced by the stake-proportional block |
fee system needs to enforce: | ||
|
||
1. **Succesful** payment at block inclusion time (implying the ability to 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.
1. **Succesful** payment at block inclusion time (implying the ability to check | |
1. **Successful** payment at block inclusion time (implying the ability to check |
1. The tx encodes a `Transfer` | ||
2. The `shielded` field must be set to `Some` | ||
3. The `source` address must be the masp4. The `target` address matches that of |
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.
what's the masp4?
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.
Typo
|
||
If checks 1 to 5 fail, the transaction can be safely discarded, while if the | ||
check fails at point 6 the transaction could be kept in mempool for 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.
what is point 6? there are only 5 points above
|
||
We provide a mapping between all the whitelisted transactions and VPs to their | ||
cost in gas units: more specifically, the cost of a tx/VP is given by the run |
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.
but this is a fixed mapping, right? the actual run-time execution speed could vary
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, for the moment it will be a fixed mapping. After this first implementation is done, we'll start working on a proper run-time gas meter
78b070f
to
70f4201
Compare
to unshield some funds on the go to cover the cost of the fee. This also | ||
addresses a possible locked-out problem in which a user doesn't have enough | ||
funds to pay fees (preventing any sort of operation on the chaind). The |
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.
funds to pay fees (preventing any sort of operation on the chaind). The | |
funds to pay fees (preventing any sort of operation on the chain). The |
|
||
1. The tx encodes a `Transfer` | ||
2. The `shielded` field must be set to `Some` |
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 probably need to limit the number of consumed and created notes, 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.
Either that or we could place a gas limit on the unshielding transaction itself, what do you think is the best approach?
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.
Perhaps both, to be honest. This is a potential DoS vector if it's non-trivial to check.
@grarco to finish this pr in the next couple of days and merge latest by mid next week |
9438b5c
to
79b5335
Compare
79b5335
to
340f12a
Compare
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.
ACK modulo minor comments
pub gas_limit: GasLimit, | ||
/// The optional unshielding transaction for fee payment | ||
pub unshield: Option<Transaction>, |
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 should be only a shielded tx, not a full Transaction
, right?
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.
Exactly, but in code the shielded (masp) part of a transfer is a struct called Transaction
``` | ||
|
||
The new `unshield` field carries an optional masp transaction struct. The |
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.
what is the type of a "MASP transaction struct"? presumably we should use that in the field above
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 above, it's the Transaction
struct, so the field is correct, it's maybe unfortunate the naming scheme that we currently have in code
* mateusz/shared-sdk-integration-wip: changelog: add #1238 wasm: update checksums feat: point to the masp_proofs with correct multicore feature flag feat: disable multicore ff by default, make ShieldedUtils trait async format: rustfmt for incorrect sdk-wallet-force commits changelog: add #925, update changelog: add #889 DoS checks in fee specs for fee unshielding ci: remove clippy-abcipp check Adjusts block proposer address in fee specs Improves unshielding tx verification in fee specs Updates tendermint link in fee specs Improves gas accounting in specs Updates check table in fee specs Misc updates to fee specs Fixes wal in fee specs Adds protocol transactions to fee specs Adds governance proposals to fee specs Fixes unshielding in fee specs Enforces tx type order in fee specs Refactors sections of fee specs Adds unshielding to fee specs Updates fee specs Adds fee specs
* brent/cubic-slashing: WIP changes from Manu and logging fixing `find_slashes_in_range` pos sm test: ease load on the CI changelog: #892 fixup!: don't call `process_slashes` within `advance_epoch` clean up logging fix clippy remove test code until slash pool transfers are solved rip slash pool get_slashed_amount: inclusive on infraction epoch add cli to sdk impl for tx unjail make find_slashes_in_ranges inclusive on end epoch withdraw: fix bounds for collecting slashes for an unbond aesthetic cleaning revert bound cleaning for readability refactor slash lookup fixup! add cubic_slash_window_length to bounds (maybe still needs change) remove unused cubic slash function refactor epoch offsets with params methods store total bond sums of each validator for efficient computation pos/lib.rs: WIP fix things inside of `bonds_and_unbonds` fix PoS client query related functions fix `bond_amount` state machine test: add slashing basic nested map test slashing: unit and e2e tests Makefile and Cargo.toml cubic and general slashing algorithms and transactions format: rustfmt for incorrect sdk-wallet-force commits changelog: add #925, update ci: remove clippy-abcipp check changelog: add #889 DoS checks in fee specs for fee unshielding Adjusts block proposer address in fee specs Improves unshielding tx verification in fee specs Updates tendermint link in fee specs Improves gas accounting in specs Updates check table in fee specs Misc updates to fee specs Fixes wal in fee specs Adds protocol transactions to fee specs Adds governance proposals to fee specs Fixes unshielding in fee specs Enforces tx type order in fee specs Refactors sections of fee specs Adds unshielding to fee specs Updates fee specs Adds fee specs
* brent/cubic-slashing: fix wasm tx tests WIP changes from Manu and logging fixing `find_slashes_in_range` pos sm test: ease load on the CI changelog: #892 fixup!: don't call `process_slashes` within `advance_epoch` clean up logging fix clippy remove test code until slash pool transfers are solved rip slash pool get_slashed_amount: inclusive on infraction epoch add cli to sdk impl for tx unjail make find_slashes_in_ranges inclusive on end epoch withdraw: fix bounds for collecting slashes for an unbond aesthetic cleaning revert bound cleaning for readability refactor slash lookup fixup! add cubic_slash_window_length to bounds (maybe still needs change) remove unused cubic slash function refactor epoch offsets with params methods store total bond sums of each validator for efficient computation pos/lib.rs: WIP fix things inside of `bonds_and_unbonds` fix PoS client query related functions fix `bond_amount` state machine test: add slashing basic nested map test slashing: unit and e2e tests Makefile and Cargo.toml cubic and general slashing algorithms and transactions format: rustfmt for incorrect sdk-wallet-force commits changelog: add #925, update ci: remove clippy-abcipp check changelog: add #889 DoS checks in fee specs for fee unshielding Adjusts block proposer address in fee specs Improves unshielding tx verification in fee specs Updates tendermint link in fee specs Improves gas accounting in specs Updates check table in fee specs Misc updates to fee specs Fixes wal in fee specs Adds protocol transactions to fee specs Adds governance proposals to fee specs Fixes unshielding in fee specs Enforces tx type order in fee specs Refactors sections of fee specs Adds unshielding to fee specs Updates fee specs Adds fee specs
Adds specs for gas and fee.
Addresses #69