[RFC][libra-framework] Refactored prologue out of LibraAccount #5881

runtian-zhou · 2020-09-02T04:22:16Z

Motivation

Moved all transaction related logic outside of LibraAccount and refactored prologue and epilogue logic into a single module.

For the LibraAccount module there will be two public apis: check_sender and charge_transaction, which is the LibraAccount related logic that is extracted out from the old prologue and epilogue.

Justification:

All transaction prologue and epilogues will now be grouped into one module. Now Block Prologue is the only function that LibraVM invokes that is outside of this module.
Cleaned up the bump_sequence_number ugliness in the writeset epilogue.
Move all Libra transaction related logic outside of LibraAccount module.
Maximize the code shared across different prologues and epilogues.

I haven't fixed the code in libra_vm invokation so tests are guarenteed to fail. Just want to know how people feel like about this rework. Will fixup tests if people are happy about this.

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

Haven't tested yet.

runtian-zhou · 2020-09-02T04:25:18Z

language/stdlib/modules/LibraTransaction.move

@@ -0,0 +1,213 @@
+address 0x1 {
+
+module LibraTransaction {


This is the newly created module for transaction prologues and epilogues (what used to be LibraWriteSetManager).

I wonder if we can come up with a name conveying that this has transaction processing business logic rather than defining the structure of transactions (e.g., there's no struct Transaction here). Maybe TransactionManager, TransactionLifecycle, TransactionProcessing?

runtian-zhou · 2020-09-02T04:26:16Z

language/stdlib/modules/LibraAccount.move

-        sender: address,
+    /// This function can only be called during the epilogue as any invokation during the execution body will cause
+    /// the sequence number to be poisoned.
+    public fun charge_transaction<Token>(


Note that although this function is public, it is not invokable from regular user scripts as it will cause the sequence number to be incremented and thus fail the assertion at line 885.

I don't understand--can't the caller choose any value they want for txn_sequence_number? E.g.

fun my_evil_function(signer: &signer) { let any_num_i_want = 7; LibraAccount::epilogue(signer, signer, 0, any_num_i_want); }

MIRAI-bot · 2020-09-02T04:34:58Z

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

jolestar · 2020-09-03T00:48:50Z

It is possible to support a new type of visibility in Move, similar to java's protected or rust's pub(crate), where the methods that use this visibility can only be called by the modules at the same address.

runtian-zhou · 2020-09-03T01:52:40Z

It is possible to support a new type of visibility in Move, similar to java's protected or rust's pub(crate), where the methods that use this visibility can only be called by the modules at the same address.

I'd really love for support like that. But IMHO we don't have a clear path towards it anytime soon as supporting such feature will actually need to change the bytecode format + bytecode verifier, which seems to be a bit heavy right now.

bob-wilson · 2020-09-04T17:59:14Z

I really like this change, but I have reservations about doing this kind of refactoring so long after our feature-complete date. What about if we put all the prologue/epilogue code into LibraAccount (where most of it is already) instead of creating a new module? It wouldn't be as nicely factored but it seems like it would achieve most of the benefits with far less churn in the code.

runtian-zhou · 2020-09-04T19:27:59Z

I really like this change, but I have reservations about doing this kind of refactoring so long after our feature-complete date. What about if we put all the prologue/epilogue code into LibraAccount (where most of it is already) instead of creating a new module? It wouldn't be as nicely factored but it seems like it would achieve most of the benefits with far less churn in the code.

I think the major concern is that we need to expose charge_transaction even if we keep the current structure of code. The only gain that buys us is that the LibraVM will remain unchanged.

bob-wilson · 2020-09-04T19:48:32Z

I think the major concern is that we need to expose charge_transaction even if we keep the current structure of code. The only gain that buys us is that the LibraVM will remain unchanged.

Why? Wouldn't all the callers be in the LibraAccount module so that charge_transaction could be private?

sblackshear · 2020-09-04T19:30:05Z

language/stdlib/modules/LibraAccount.move

+    /// This function can only be called during the epilogue as any invokation during the execution body will cause
+    /// the sequence number to be poisoned.
+    public fun charge_transaction<Token>(
+        vm_signer: &signer,


unused? Did you mean to assert Signer::address_of(vm_signer) == VM_RESERVED_ADDRESS?

sblackshear · 2020-09-04T19:31:05Z

language/stdlib/modules/LibraAccount.move

-        sender: address,
+    /// This function can only be called during the epilogue as any invokation during the execution body will cause
+    /// the sequence number to be poisoned.
+    public fun charge_transaction<Token>(


I don't understand--can't the caller choose any value they want for txn_sequence_number? E.g.

fun my_evil_function(signer: &signer) { let any_num_i_want = 7; LibraAccount::epilogue(signer, signer, 0, any_num_i_want); }

sblackshear · 2020-09-04T21:12:00Z

language/stdlib/modules/LibraTransaction.move

+    ) {
+        assert(
+            LibraTransactionPublishingOption::is_module_allowed(sender),
+            PROLOGUE_EMODULE_NOT_ALLOWED


Did we decide to use Errors here as well? @tnowacki @tzakian

#5876

I have a PR up to use Errors everywhere. We should probably get that in first

sblackshear · 2020-09-04T22:52:23Z

language/stdlib/modules/LibraTransaction.move

+
+    /// The common prologue is invoked at the beginning of every transaction
+    /// It verifies:
+    /// - The account's auth key matches the transaction's public key


Add a comment about chain_id?

sblackshear · 2020-09-04T22:52:40Z

language/stdlib/modules/LibraTransaction.move

+    /// It verifies:
+    /// - The account's auth key matches the transaction's public key
+    /// - That the account has enough balance to pay for all of the gas
+    /// - That the sequence number matches the transaction's sequence key


Suggested change

/// - That the sequence number matches the transaction's sequence key

/// - That the account's sequence number matches the transaction's sequence number

sblackshear · 2020-09-04T23:29:54Z

language/stdlib/modules/LibraTransaction.move

+        assert(sender == CoreAddresses::LIBRA_ROOT_ADDRESS(), PROLOGUE_EINVALID_WRITESET_SENDER);
+
+        // Currency code don't matter here as it won't be charged anyway.
+        LibraAccount::check_sender<LBR::LBR>(account, 0, writeset_sequence_number, writeset_public_key);


Should we call prologue_common here instead of this to resolve the TODO below?

sblackshear · 2020-09-04T23:31:36Z

language/stdlib/modules/LibraTransaction.move

+    ) {
+        assert(sender == CoreAddresses::LIBRA_ROOT_ADDRESS(), PROLOGUE_EINVALID_WRITESET_SENDER);
+
+        // Currency code don't matter here as it won't be charged anyway.


This + the 0 gas price feels a bit icky and is making me wonder whether we want separate LibraAccount::gas_prologue and LibraAccount::replay_prevention_prologues (or similar...)

sblackshear · 2020-09-04T23:36:53Z

language/stdlib/modules/LibraTransaction.move

+        // Charge for gas
+        assert(txn_max_gas_units >= gas_units_remaining, Errors::invalid_argument(EGAS));
+        let gas_used = txn_max_gas_units - gas_units_remaining;
+        assert((txn_gas_price as u128) * (gas_used as u128) <= MAX_U64, Errors::limit_exceeded(EGAS));


Maybe do assert(txn_gas_price <= MAX_U64 / gas_used) to avoid the u128 conversions?

sblackshear · 2020-09-04T23:37:42Z

language/stdlib/modules/LibraTransaction.move

+    ) {
+        let sender = Signer::address_of(account);
+
+        // Charge for gas


I find this comment confusing given that the actual charging happens in LibraAccount. This part is just sanity-checking the gas price and gas unit values.

sblackshear · 2020-09-04T23:38:11Z

language/stdlib/modules/LibraTransaction.move

+
+    /// The failure_epilogue is invoked at the end of transactions when the transaction is aborted during execution or
+    /// during `success_epilogue`.
+    fun failure_epilogue<Token>(


Agreed with you that these are now the same and we can kill one of them.

tnowacki · 2020-09-08T18:22:00Z

Also very worried about making a change like this so late.

Why? Wouldn't all the callers be in the LibraAccount module so that charge_transaction could be private?

Do we have an answer on this? I think that if we don't need to expose new public APIs, we shouldn't do it. Even if it means more code duplication.
In short, I don't really yet buy many of the motivations outlined in the summary, but could be convinced otherwise

Additionally, we should really re-evaluate this after #5876. There would only need to be a single epilogue in LibraAccount and might help us look at things differently

[libra-framework] Refactored prologue out of LibraAccount

2028e60

runtian-zhou added the breaking Any changes that will require restarting the testnet label Sep 2, 2020

runtian-zhou requested review from tzakian, sblackshear, dariorussi-zz, shazqadeer, vgao1996, wrwg and bob-wilson September 2, 2020 04:22

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

runtian-zhou marked this pull request as draft September 2, 2020 04:24

runtian-zhou commented Sep 2, 2020

View reviewed changes

sblackshear reviewed Sep 4, 2020

View reviewed changes

runtian-zhou mentioned this pull request Sep 15, 2020

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

Merged

runtian-zhou closed this Oct 8, 2020

bors-libra removed this from In Review in bors Oct 8, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RFC][libra-framework] Refactored prologue out of LibraAccount #5881

[RFC][libra-framework] Refactored prologue out of LibraAccount #5881

runtian-zhou commented Sep 2, 2020 •

edited

runtian-zhou Sep 2, 2020

sblackshear Sep 4, 2020

runtian-zhou Sep 2, 2020

sblackshear Sep 4, 2020

MIRAI-bot commented Sep 2, 2020

jolestar commented Sep 3, 2020

runtian-zhou commented Sep 3, 2020

bob-wilson commented Sep 4, 2020

runtian-zhou commented Sep 4, 2020

bob-wilson commented Sep 4, 2020

sblackshear Sep 4, 2020

sblackshear Sep 4, 2020

sblackshear Sep 4, 2020

tnowacki Sep 8, 2020

sblackshear Sep 4, 2020

sblackshear Sep 4, 2020

sblackshear Sep 4, 2020

sblackshear Sep 4, 2020

sblackshear Sep 4, 2020

sblackshear Sep 4, 2020

sblackshear Sep 4, 2020

tnowacki commented Sep 8, 2020 •

edited

	/// - That the sequence number matches the transaction's sequence key
	/// - That the account's sequence number matches the transaction's sequence number

[RFC][libra-framework] Refactored prologue out of LibraAccount #5881

[RFC][libra-framework] Refactored prologue out of LibraAccount #5881

Conversation

runtian-zhou commented Sep 2, 2020 • edited

Motivation

Have you read the Contributing Guidelines on pull requests?

Test Plan

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

MIRAI-bot commented Sep 2, 2020

jolestar commented Sep 3, 2020

runtian-zhou commented Sep 3, 2020

bob-wilson commented Sep 4, 2020

runtian-zhou commented Sep 4, 2020

bob-wilson commented Sep 4, 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

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

tnowacki commented Sep 8, 2020 • edited

runtian-zhou commented Sep 2, 2020 •

edited

tnowacki commented Sep 8, 2020 •

edited