[vm] Introducing a delta write #2057
Conversation
georgemitenkov
requested review from
davidiw,
msmouse,
wrwg,
gelash and
zekun000
|
Looks good to me, Two notes:
|
|
fyi: We call this |
| pub enum WriteOp { | ||
| Deletion, | ||
| Delta(DeltaOperation, DeltaLimit, Vec<u8>), |
There was a problem hiding this comment.
what is the last Vec<u8>? is it the actual delta value? I thought it's something like u128.
There was a problem hiding this comment.
we probably should also put it in the end, and having #[serde(skip)] since this is not expected to go into storage
There was a problem hiding this comment.
yes, it is the value of the delta and actually having u128 makes sense since we don't go to storage indeed
georgemitenkov
force-pushed
the
george/aggregator-delta-writes
branch
from
3ea1ed1 to
19a88fd
Compare
types/src/write_set.rs
Outdated
| #[derive(Clone, Eq, Hash, PartialEq, Serialize, Deserialize)] | ||
| pub enum WriteOp { | ||
| Deletion, | ||
| #[serde(skip)] | ||
| Delta(DeltaOperation, DeltaLimit, u128), |
There was a problem hiding this comment.
is there a reason why you don't carry u128 in the DeltaOperation? like
enum DeltaOperation {
Addition(u128),
Subtraction(u128),
}
There was a problem hiding this comment.
Initially, the idea was to have delta(op, bytes) so that any update in the future cam simply specify op and serialize value into bytes. So no particular reason of keeping u128 in Delta. If we move it to operation as you suggested, I think we should move the limit as well since it is a postcondition of DeltaOperation
There was a problem hiding this comment.
I think we should merge the u128, I don't have strong opinion on DeltaLimit since it at least has a name 😄
There was a problem hiding this comment.
Cool, let's merge u128 only then for now :)
|
It feels bad we have to leak the delta concept to outside of the VM adaptor. I advocate changing the ChangeSet, TransactionOutput (used internally by the VM adaptor) etc concepts to carry the set of delta directly, and output the final WriteSet / TransactionOutput in their original format. The fact we are now doing all the panics in all the matches all the way to the Rest API indicates it's not right. @gelash and I talked offline and agreed it's probably the best to land this first and follow up really soon to correct it. |
❌ Forge test failureForge is land-blocking |
❌ Forge test failureForge is land-blocking |
✅ Forge test successForge is land-blocking |
Fixes #2057. Now deltas do not escape from executor, and are stored in `TransactionOutputExt` and `ChangeSetExt` wrappers.
…#2222) Fixes aptos-labs#2057. Now deltas do not escape from executor, and are stored in `TransactionOutputExt` and `ChangeSetExt` wrappers.
Description
This PR introduces a new
WriteOp-Delta. Delta is parametrised by a serialised value, operation used to apply this value, and a limit (basically a postcondition that ensures the result of delta application does not overflow).Currently, deltas are not used and not produced anywhere. This PR allows aggregator from #1836 (once it is rebased and cleaned) to produce a change set containing deltas.
Test Plan
Subsequent PRs will add sequential and parallel execution support, with appropriate tests.
This change is