[breaking][libra-framework] Refactor writeset prologue and epilogue #6042

runtian-zhou · 2020-09-15T20:32:39Z

Motivation

Refactored writeset prologue and epilogue by merging the LibraWriteSetManager module into LibraAccount. This will make the prologue/epilogue code more modular. Also added checks for chain_id and expiration time for WriteSet Transaction.

This change is a breaking change.

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

cargo test

Related PRs

This is a less aggressive version of #5881

runtian-zhou · 2020-09-15T20:33:19Z

language/libra-vm/src/libra_transaction_executor.rs

@@ -443,22 +443,6 @@ impl LibraVM {
            return Ok((e, discard_error_output(StatusCode::REJECTED_WRITE_SET)));
        };

-        // Bump the sequence number of sender.


By refactoring the epilogue we can get rid of this piece of ugly code.

runtian-zhou · 2020-09-15T20:34:21Z

types/src/block_metadata.rs

@@ -69,7 +69,7 @@ impl BlockMetadata {
 }

 pub fn new_block_event_key() -> EventKey {
-    EventKey::new_from_address(&libra_root_address(), 20)
+    EventKey::new_from_address(&libra_root_address(), 21)


This is an unforunate consequnce of this change as we changed the order of how event handles are generated. Do we want to keep the same magic value?

Update: I got rid of this change by reorganizing the genesis intialization code, as this will also change the event key for a number of other system events (all mint related events).

runtian-zhou · 2020-09-15T20:35:31Z

language/stdlib/modules/LibraAccount.move

+            UpgradeEvent { writeset_payload },
+        );
+        // Currency code don't matter here as it won't be charged anyway.
+        epilogue<LBR::LBR>(lr_account, writeset_sequence_number, 0, 0, 0);


The 0 uglyness still resides. Do we want to make clean separation of incrementing sequence number only? IMHO having multiple function will introduce extra borrow_global. Do we want to trade that for the code cleaness?

I think it's fine like this. Keep it simple....

dariorussi-zz · 2020-09-16T16:10:32Z

@bob-wilson @sblackshear @tzakian @tnowacki you all have looked at and talked about this quite a bit.
If this has to go in, it would be great to get it in asap.
So if you could comment that would be great!
thanks

bob-wilson · 2020-09-16T19:36:26Z

language/stdlib/modules/LibraAccount.move

+    /// The prologue for WriteSet transaction
+    fun writeset_prologue(
+        sender: &signer,
+        writeset_sequence_number: u64,


Can we name these "txn_sequence_number" and "txn_public_key" for consistency with the other prologue functions? The account has a single sequence number shared for all kinds of transactions, right?

language/stdlib/modules/LibraAccount.move

bob-wilson · 2020-09-16T19:54:23Z

language/stdlib/modules/LibraAccount.move

+            UpgradeEvent { writeset_payload },
+        );
+        // Currency code don't matter here as it won't be charged anyway.
+        epilogue<LBR::LBR>(lr_account, writeset_sequence_number, 0, 0, 0);


I think it's fine like this. Keep it simple....

language/stdlib/modules/LibraAccount.move

runtian-zhou · 2020-09-16T20:25:16Z

Added back the specs from the old module. Renamed writeset_sequence_number to transaction_sequence_number

MIRAI-bot · 2020-09-16T20:40:33Z

[move-prover] Move Prover test (language/move-prover/cargo test) on commit 18a80b4 did fail. See details.

tzakian

Overall this looks good to me. A couple nits here and there.

The main thing I think we should consider here is combining the two singleton resources in this module (LibraWriteSetManager and AccountOperationsCapability). I would be in favor of combining them unless there are reasons that I'm not aware of.

tzakian · 2020-09-16T20:19:11Z

language/stdlib/modules/LibraAccount.move

@@ -188,6 +201,25 @@ module LibraAccount {
        );
    }

+    public fun initialize_writeset_manager(


Could this be combined with the initialize function for this module? It seems like a cleaner approach to include this with that.

Yesss! There's a slight gotcha here: the order of how event handles are generated are depending on the order of calls in the genesis module. If we merge those functions, the event key for a number of functions will be modified. The most important one of them is probably the EventKey for new_epoch_event will be off by one, which will be a "bigger" change. Some of the json-rpc tests also hard coded those event keys. That's why I keep the two resource separate and have two init functions so that the order of creating event handles could be perserved.

Avoiding churn in the EventKeys seems worth it to have 2 init functions. My $0.02

There is already a breaking PR that changes the initialization sequence of events and changes these event keys (see Davids SEV post in libra core). It might be worthwhile to try and coordinate this with that PR, and then we could combine these and have only one break of the event keys?

tzakian · 2020-09-16T20:29:05Z

language/stdlib/modules/LibraAccount.move

@@ -79,10 +80,18 @@ module LibraAccount {
        limits_cap: AccountLimitMutationCapability,
    }

+    /// A resource for the number of WriteSets that has been committed on chain.


What do you think of including this in the AccountOperationsCapability (and possibly renaming to something more descriptive)? It's a singleton resource as well, so seems like it would be a good fit.

The comment also seems opaque: "number of Writesets" doesn't really make sense to me. Please update to clarify.

Certainly the comment should be fixed up, but at this point, I'd say that it would be best to minimize the change and keep it as a separate resource, just from the perspective of reducing risk and churn in the code. I could be convinced otherwise if you really think it is worth doing....

Good question about churn, and I would agree if this weren't already a breaking change...

I guess my opinion here is that since it is a breaking change already that we should combine these since combining them is no more "breaking" than moving the LibraWriteSetManager resource which is being done in this PR. This will need to be a genesis reset either way.

I defer to your expertise but please be cautious....

language/stdlib/modules/LibraAccount.move

tzakian · 2020-09-16T21:38:26Z

cc @shazqadeer in case you're working on the prologue

MIRAI-bot · 2020-09-16T22:24:40Z

[move-prover] Move Prover test (language/move-prover/cargo test) on commit 984bbcb did fail. See details.

runtian-zhou · 2020-09-16T22:48:18Z

Talked to @tzakian over the genesis initialization seuqnece issue. It seems to us that we are hitting similar issue as #5997. So if both PR can land in the same release cycle then we should aim for merging the two resources/init function.

MIRAI-bot · 2020-09-16T23:17:18Z

[move-prover] Move Prover test (language/move-prover/cargo test) on commit 63cc29b did fail. See details.

tnowacki · 2020-09-18T18:55:51Z

language/stdlib/modules/LibraAccount.move

+        );
+        assert(Roles::has_libra_root_role(sender), Errors::invalid_argument(PROLOGUE_INVALID_WRITESET_SENDER));
+
+        // Currency code don't matter here as it won't be charged anyway. Gas constants are ommitted.


I could go either way on this but...

It feels a bit bizarre to me to that we are using a "common" prologue function here when a few of the checks aren't common.
I would want it to be extraordinarily clear what checks are being done in each prologue. Passing in 0's and then saying we ignoring type arguments feels brittle to me. And I feel might be confusing. I feel the goal of these prologue functions should be to make it 100% clear what checks are being performed on each transaction type.

So counter proposal here, split prologue common into separate verification/check passes
The existing calls to prologue_common would be something like

prologue_check_account_data(...); prologue_check_gas_payment<TOKEN>(...); prologue_check_sequence_number(...);

Then for the writeset check here, we would have

prologue_check_account_data(...); prologue_check_sequence_number(...);

tnowacki · 2020-09-18T18:58:08Z

language/stdlib/modules/LibraAccount.move

+            UpgradeEvent { writeset_payload },
+        );
+        // Currency code don't matter here as it won't be charged anyway.
+        epilogue<LBR::LBR>(lr_account, txn_sequence_number, 0, 0, 0);


Same comment above about splitting off checks. I'd prefer to see separate functions then one centralized function with a bunch of dummy arguments

Existing epilogue would contain

fun epilogue<...>(...) { epilogue_deduct_gas_payment(...) epilogue_bump_sequence_number(...) }

I do not like having one central function and then having to jump through hoops to convince ourselves it is safe

I'm a bit torn over here. On one hand I love this idea of having separate functions we can call to get the logics we want. On the other hand, I'm trying to minimize the code change and keep the code structure as it was (and try to merge it ASAP as soon as #5997 lands to leverage the fact that it is touching the genesis init order). I'm also wasn't sure how it might affect the prover side work. Do you have any strong preference over here? cc @tzakian @sblackshear @shazqadeer for thoughts as well.

I feel like splitting it up might make the prover's life easier? beats me though
If its seems like work to convince a human that the checks aren't happening, I feel like it might be cumbersome to convey this in the spec as well, but I could be wrong

I don't think splitting it would make the prover's life easier (but, it's one of those things where it's hard to determine exactly what will make it harder/easier until you start proving it). I think for now in the interest of getting the breaking change in for this release cycle (cut tomorrow morning) I would like to hold off on this, and if we do decide to refactor it out we can later on and that shouldn't be a breaking change.

tnowacki · 2020-09-18T19:00:55Z

language/stdlib/modules/LibraAccount.move

@@ -1121,6 +1198,14 @@ module LibraAccount {
        // Check that the chain ID stored on-chain matches the chain ID specified by the transaction
        assert(ChainId::get() == chain_id, Errors::invalid_argument(PROLOGUE_EBAD_CHAIN_ID));

+        // Check that the gas calculation will not overflow


Why was this check moved?

Actually I shouldn't move them. This was coming from my previous version of refactor where I had the gas related function outside of prologue_common

runtian-zhou · 2020-09-20T23:45:36Z

Interestingly, when bundled this PR with #5997, it reverted the change to json-rpc tests as the orders are still preserved. However there's still changes done to new_epoch_event and new_block_event which still made this PR a "hard" breaking change.

MIRAI-bot · 2020-09-20T23:57:00Z

[move-prover] Move Prover test (language/move-prover/cargo test) on commit 92df18b did fail. See details.

MIRAI-bot · 2020-09-21T00:12:12Z

[move-prover] Move Prover test (language/move-prover/cargo test) on commit 47221b9 did fail. See details.

bob-wilson · 2020-09-22T18:39:11Z

@tzakian It sounds like you're OK with the current version, is that right? I'm pretty happy with it now, and if we're going with the initialization change that you wanted, we should land this ASAP so it goes along with #5997

tnowacki · 2020-09-22T18:43:24Z

@tzakian @bob-wilson Is the current version with what is checked in now? In that case I'm happy with that too.

It looks like there were two checks added to the writeset prologue and the VM? Maybe we should just do those two changes as a smaller PR and get them in ASAP

bob-wilson · 2020-09-22T18:48:22Z

@tzakian @bob-wilson Is the current version with what is checked in now? In that case I'm happy with that too.

I'm not sure what you're asking. I was referring to the current contents of this PR. I think it's ready to land.

It looks like there were two checks added to the writeset prologue and the VM? Maybe we should just do those two changes as a smaller PR and get them in ASAP

Yes, this fixes #5708 by adding checks for the ChainID and txn expiration. I'd much prefer to just land this one. Otherwise, if this doesn't go in along with #5997, I think we should go back to a separate initialization function to preserve the event keys, and Tim really didn't like like that.

tzakian · 2020-09-22T19:56:16Z

@tzakian It sounds like you're OK with the current version, is that right? I'm pretty happy with it now, and if we're going with the initialization change that you wanted, we should land this ASAP so it goes along with #5997

Yep, I'm good with this going in as-is. Agreed on getting this in ASAP (need to land before tomorrow morning!)

tzakian · 2020-09-22T20:28:40Z

/help

bors-libra · 2020-09-22T20:28:42Z

Bors help and documentation

Bors is a Github App used to manage merging of PRs in order to ensure that CI is always green. It does so by maintaining a Merge Queue. Once a PR reaches the head of the Merge Queue it is rebased on top of the latest version of the PR's base-branch (generally master) and then triggers CI. If CI comes back green the PR is then merged into the base-branch. Regardless of the outcome, the next PR is the queue is then processed.

General

This project's Merge Queue can be found here.
This project requires PRs to be Reviewed and Approved before they can be queued for merging.
Before PR's can be merged they must be configured with "Allow edits from maintainers" enabled. This is needed for Bors to be able to update PR's in-place so that Github can properly recognize and mark them as "merged" once they've been merged into the upstream branch.

Commands

Bors actions can be triggered by posting a comment which includes a line of the form /<action>.

Command	Action	Description
Land	`land`, `merge`	attempt to land or merge a PR
Canary	`canary`, `try`	canary a PR by performing all checks without merging
Cancel	`cancel`, `stop`	stop an in-progress land
Cherry Pick	`cherry-pick <target>`	cherry-pick a PR into `<target>` branch
Priority	`priority`	set the priority level for a PR (`high`, `normal`, `low`)
Help	`help`, `h`	show this help message

Options

Options for Pull Requests are configured through the application of labels.

Option	Description
	Indicates that the PR is high-priority. When queued the PR will be placed at the head of the merge queue.
	Indicates that the PR is low-priority. When queued the PR will be placed at the back of the merge queue.
	Before merging the PR will be squashed down to a single commit, only retaining the commit message of the first commit in the PR.

tzakian · 2020-09-22T20:29:18Z

/land

Closes: diem#6042

github-actions · 2020-09-22T21:17:53Z

Cluster Test Result

all up : 1014 TPS, 4457 ms latency, 5300 ms p99 latency, no expired txns
Logs: http://kibana.ct-0-k8s-testnet.aws.hlw3truzy4ls.com/app/kibana#/discover?_g=(time:(from:'2020-09-22T21:05:29Z',to:'2020-09-22T21:17:52Z'))
Dashboard: http://grafana.ct-0-k8s-testnet.aws.hlw3truzy4ls.com/d/2XqUIhnWz/performance?from=1600808729000&to=1600809472000
Validator 1 logs: http://kibana.ct-0-k8s-testnet.aws.hlw3truzy4ls.com/app/kibana#/discover?_g=(time:(from:'2020-09-22T21:05:29Z',to:'2020-09-22T21:17:52Z'))&_a=(columns:!(log),query:(language:kuery,query:'kubernetes.pod_name:"val-1"'),sort:!(!('@timestamp',desc)))

Repro cmd:

./scripts/cti --tag land_ce88eb42 -E RUST_LOG=debug --report report.json --suite land_blocking

🎉 Land-blocking cluster test passed! 👌

runtian-zhou added the language Changes to the Move language. Includes any crate in the `language` subdirectory label Sep 15, 2020

runtian-zhou requested review from tnowacki, tzakian, sblackshear, dariorussi-zz, vgao1996 and bob-wilson September 15, 2020 20:32

bors-libra added this to In Review in bors Sep 15, 2020

runtian-zhou commented Sep 15, 2020

View reviewed changes

runtian-zhou force-pushed the refactor_prologue_epilouge branch 2 times, most recently from 0bd7372 to 74ffffd Compare September 16, 2020 02:20

diem deleted a comment from MIRAI-bot Sep 16, 2020

runtian-zhou force-pushed the refactor_prologue_epilouge branch 2 times, most recently from a9ff90e to 035ea01 Compare September 16, 2020 08:26

bob-wilson suggested changes Sep 16, 2020

View reviewed changes

tzakian requested a review from shazqadeer September 16, 2020 20:01

tzakian reviewed Sep 16, 2020

View reviewed changes

runtian-zhou force-pushed the refactor_prologue_epilouge branch from 63cc29b to 2153e46 Compare September 16, 2020 23:41

diem deleted a comment from MIRAI-bot Sep 17, 2020

tnowacki reviewed Sep 18, 2020

View reviewed changes

runtian-zhou force-pushed the refactor_prologue_epilouge branch from 2153e46 to 1cabb18 Compare September 20, 2020 23:13

runtian-zhou force-pushed the refactor_prologue_epilouge branch from 92df18b to 47221b9 Compare September 20, 2020 23:54

tzakian requested a review from bob-wilson September 21, 2020 22:04

tzakian force-pushed the refactor_prologue_epilouge branch 2 times, most recently from 2deba20 to af9c2a2 Compare September 22, 2020 15:52

tzakian changed the title ~~[RFC][breaking][libra-framework] Refactor writeset prologue and epilogue~~ [breaking][libra-framework] Refactor writeset prologue and epilogue Sep 22, 2020

tzakian force-pushed the refactor_prologue_epilouge branch 2 times, most recently from 510febb to b6896c0 Compare September 22, 2020 17:25

bob-wilson approved these changes Sep 22, 2020

View reviewed changes

bors-libra moved this from In Review to Queued in bors Sep 22, 2020

[breaking][libra-framework] Refactor writeset prologue and epilogue

ce88eb4

Closes: diem#6042

bors-libra moved this from Queued to Testing in bors Sep 22, 2020

bors-libra force-pushed the refactor_prologue_epilouge branch from b6896c0 to ce88eb4 Compare September 22, 2020 21:19

bors-libra removed this from Testing in bors Sep 22, 2020

bors-libra closed this in ce88eb4 Sep 22, 2020

bors-libra merged commit ce88eb4 into diem:master Sep 22, 2020

sausagee added the breaking Any changes that will require restarting the testnet label Sep 22, 2020

bob-wilson mentioned this pull request Sep 24, 2020

LibraWriteSetManager::prologue needs to check ChainId and expiration time #5708

Closed

sblackshear mentioned this pull request Sep 25, 2020

[breaking][libra framework] add event on account creation #6156

Merged

[breaking][libra-framework] Refactor writeset prologue and epilogue #6042

[breaking][libra-framework] Refactor writeset prologue and epilogue #6042

Conversation

runtian-zhou commented Sep 15, 2020

Motivation

Have you read the Contributing Guidelines on pull requests?

Test Plan

Related PRs

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dariorussi-zz commented Sep 16, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

runtian-zhou commented Sep 16, 2020

MIRAI-bot commented Sep 16, 2020

tzakian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tzakian commented Sep 16, 2020

MIRAI-bot commented Sep 16, 2020

runtian-zhou commented Sep 16, 2020

MIRAI-bot commented Sep 16, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tzakian Sep 22, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

runtian-zhou commented Sep 20, 2020

MIRAI-bot commented Sep 20, 2020

MIRAI-bot commented Sep 21, 2020

bob-wilson commented Sep 22, 2020

tnowacki commented Sep 22, 2020

bob-wilson commented Sep 22, 2020

tzakian commented Sep 22, 2020

tzakian commented Sep 22, 2020

bors-libra commented Sep 22, 2020

General

Commands

Options

tzakian commented Sep 22, 2020

github-actions bot commented Sep 22, 2020

tzakian Sep 22, 2020 •

edited