Add TransactionExpiration::Validity with allowed proposers - #27454
Add TransactionExpiration::Validity with allowed proposers#27454mystenmark wants to merge 1 commit into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
b3ce558 to
e9a06b6
Compare
e9a06b6 to
2434c0b
Compare
GitHub Actions
Inactive
| }, | ||
| /// Everything in `ValidDuring`, plus a restriction on which validators may propose the | ||
| /// transaction in consensus. | ||
| Validity { |
There was a problem hiding this comment.
Eventually we want this to be the default choice for every submitted transaction right? Every other choice will lead to a transaction being submitted without any proposers, but potentially go down a throttled path (per our latest discussion).
There was a problem hiding this comment.
yes, if this works well in practice we would want to move towards this as the default
| /// User-provided uniqueness identifier to differentiate otherwise identical transactions | ||
| nonce: u32, | ||
| /// Indices into the current epoch's committee of the validators allowed to propose this | ||
| /// transaction in consensus. Must be strictly increasing. Proposal by any other validator |
There was a problem hiding this comment.
Why increasing? I thought we wanted to give some flexibility to the user (well the FN) to optimise the choices here.
There was a problem hiding this comment.
they can choose whatever validators they want, this list should just be ordered
| @@ -795,6 +795,10 @@ impl ValidatorService { | |||
| let tx_size = transaction.validity_check(&epoch_store.tx_validity_check_context())?; | |||
There was a problem hiding this comment.
I guess this will be address on a separate PR as this focuses on the proposer direction, but we do need to check the validity of the proposers list (no duplicates, not bigger than the committee size, respect the SIP-45 etc)
2434c0b to
c061d35
Compare
c061d35 to
4d384d3
Compare
4d384d3 to
14e7c0e
Compare
14e7c0e to
726e763
Compare
GitHub Actions
Inactive
There was a problem hiding this comment.
Should we also be exposing the allowed proposers as its own field?
There was a problem hiding this comment.
what do you think? I would assume this wouldn't be of interest for most indexer consumers
There was a problem hiding this comment.
We tend to provide a faithful representation of everything in the protocol as structured fields. It's not necessarily helpful for apps, but it's quite helpful for tools/explorers etc.
There was a problem hiding this comment.
Doesn't have to block this PR though, it can be a follow-up.
| /// | ||
| /// Naming several proposers is also what keeps a transaction submittable when some of them are | ||
| /// offline, so this is a floor as much as it is an allowance. | ||
| pub const MAX_UNPAID_ALLOWED_PROPOSERS: u64 = 3; |
There was a problem hiding this comment.
Thinking of the flow here, and I am totally find to defer that conversation once the transaction_driver changes land, once the transaction is submitted to the FN, the transaction_driver will submit only to the allowed validators - with some retry logic - unless we are quite unfortunate and all the proposers are unavailable, where in this case we might need the user to re-sign a transaction with a new set of proposers. Is this how you are thinking of it?
There was a problem hiding this comment.
exactly. I think it would be very unlikely to have all 3 proposers offline, since we will be getting the preferred validator list from transaction driver. there would have to be a small window where tx driver prefers validators A, B, and C, they are added as allowed proposers, and by the time the tx returns to the fullnode, all of A B & C have gone down.
There was a problem hiding this comment.
If this becomes a problem in practice, my thought was that we could allow submission from any validator, but with a delay.
There was a problem hiding this comment.
In practice I can only see this happening during a network upgrade, where validators update their binaries with some downtime, and this taking a bit of time to robustly detect from the transaction_driver, or during some incidents (the more edge case scenario here). I believe allowing submission by any validator with a delay could be a good alternative here.
| // An out-of-range index names no one, so it can only make the transaction | ||
| // unproposable. | ||
| if let Some(out_of_range) = | ||
| proposers.iter().find(|i| **i >= context.committee_size) |
There was a problem hiding this comment.
I know that both this and previous check cover the length check, but maybe I would put an explicit length check first and ensure no search, sort check etc is done for a malicious case:
fp_ensure!(
proposers.len() <= context.comittee_size,
UserInputError::InvalidExpiration {
error: "allowed_proposers length greater than committee size".to_string(),
}
.into()
);
There was a problem hiding this comment.
Alternatively you could bring up the max_proposers check and also cap by:
let amplification = self.gas_data.price / context.reference_gas_price.max(1);
let max_proposers = (MAX_UNPAID_ALLOWED_PROPOSERS.max(amplification)).min(context.comittee_size);
| // A transaction that names its proposers has already had its amplification bounded by | ||
| // validity_check, which sizes the proposer set against the gas price, so there is nothing | ||
| // left to charge for here. A set recorded for another epoch is ignored and so bounds | ||
| // nothing, leaving the transaction subject to deferral like any other. |
There was a problem hiding this comment.
A validator can still propose multiple copies of the same transaction in the same commit - but I guess we don't want to penalise in this case the transaction/user it self right? Maybe we could still capture some metrics to detect "potential" deferrals - which will practically reveal validator byzantine behaviour?
There was a problem hiding this comment.
good point. i'm not even sure it requires byzantine behavior, the existing code may allow duplicate submissions under certain circumstances (such as during retries).
however, i don't think a validator could get a latency edge via its own duplicate submissions, so there doesn't seem to be much incentive to do this.
There was a problem hiding this comment.
Yes overall I agree, that's why I suggested only just keeping the metrics part - not the deferral.
Regarding the latency edge, well a validator proposing a block at round R can only be certain that their transaction will get committed at minimum after 2 rounds (assuming they were the leader at R). There can be other tricks - assumptions so they can have some probabilistically high certainty, but that could be the baseline. So yes, I don't think it makes any sense for the to amplify in the same block of course, but maybe for 2-3 rounds overall they could. Whether the duplication will be detectable in the same commit, that we can't tell , but in the same time we might be talking for a edge case.
To conclude, I agree with disabling the deferral mechanism, but having some insight on the behaviour is what I would look to keep.
726e763 to
925275a
Compare
GitHub Actions
Inactive
Adds a transaction expiration variant carrying everything in ValidDuring plus the committee indices of the validators allowed to propose the transaction in consensus, behind the `allowed_proposers` protocol feature flag (devnet only). Enforced in two places: at admission, where the validator rejects a transaction it may not propose, and at block verification, where proposal by a disallowed validator is byzantine behavior and invalidates the whole block. The latter required threading the block ref through TransactionVerifier::verify_batch.
925275a to
3dabfd2
Compare
GitHub Actions
Active
4 participants
Description
Lets a transaction name the validators that are allowed to propose it in consensus, so a sender can pin where its transaction enters the DAG.
TransactionExpiration::Validitycarries everything inValidDuringplus anAllowedProposersset of committee indices. It is rejected at admission by any validator not in the set, and rejected at block verification if it appears in a block authored by one — the latter fails the whole block, since it is byzantine behavior. Gated on the newallowed_proposersprotocol flag, which is on for devnet only.Naming several proposers amplifies the transaction's consensus cost, so per SIP-45 the set size is bounded by
max(3, gas_price / RGP). The floor of 3 is what keeps a transaction submittable when some of its proposers are offline. Since that bound is enforced at signing time, proposer-restricted transactions are exempt from the unpaid-amplification deferral in the consensus handler — but only when the set is non-empty, as an empty set permits every validator and so bounds nothing.AllowedProposersrecords the epoch its indices refer to. That field is currently ignored — indices always resolve against the committee of the epoch the transaction is proposed in — but a validity window can span two epochs, so recording it lets us fix that later without another format change.Depends on MystenLabs/sui-rust-sdk#293 and MystenLabs/sui-apis#31 for the wire format. Until those land and the rev is bumped, proto and SDK conversions downgrade a
Validityexpiration toValidDuringand drop the proposer set (marked with TODOs), so the variant is only reachable by submitting fully-formed BCS transactions.Test plan
New tests in
sui-typescover the feature gate, the strictly-increasing requirement, the committee-size bound, and the gas-price bound.consensus_validator::tests::test_reject_disallowed_proposerbuilds a 4-validator committee and checks that a block from each non-allowed author is rejected and the allowed one is accepted;submit_transaction_tests::test_submit_transaction_disallowed_proposercovers the admission path.Not covered: a real block being rejected by peers in a running network.
Release notes
Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required.
For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates.
TransactionExpiration::Validity, which restricts the transaction to a named set of consensus proposers. Behind theallowed_proposersfeature flag, enabled on devnet only; no change on mainnet or testnet.