[breaking][libra framework] Remove LibraTransactionTimeout module #5674

sblackshear · 2020-08-19T17:12:10Z

The only purpose of LibraTransactionTimeout was to an enforce upper and lower bounds on transaction expiration time. The lower bound check is very simple to do and the upper bound check is no longer a requirement, so we can kill this module.

The PR deletes LibraTransactionTimeout and moves the lower bound check into the transaction prologue. To keep the check simple, it adds a LibraTimestamp::now_seconds function that can be directly compared against the transaction expiration time (which is specified in seconds).

This is a breaking change in the sense that it removes the LibraTransactionTimeout module and orphans the TTL resource, but it is not observable to clients and should be deployable via a WriteSet.

sblackshear

@wrwg: there are some other TODOs in LibraTimestamp like

/// > TODO(wrwg): we MUST have a capability which only tests can have to be able to call
/// > this function before we go to production
public fun reset_time_has_started_for_test() acquires TimeHasStarted {

; let me know if you want me to take care of them as long as I'm already doing a breaking change here.

bors-libra · 2020-08-19T17:14:39Z

❗ Invalid command

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.

MIRAI-bot · 2020-08-19T17:27:03Z

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

bob-wilson

I like this a lot! It's better to do the comparison in seconds instead of microseconds, since the expiration is specified in terms of seconds, and there's no risk of overflow this way.

riyazdf · 2020-08-20T16:20:08Z

noted, thanks!

n4ss · 2020-08-20T16:31:50Z

Looks good!

github-actions · 2020-08-20T16:36:52Z

release binaries dependency change summary:

target packages:
  M serde-generate 0.11.3 (direct third-party, crates.io)
    * version upgraded from 0.11.0
    * (unchanged features: [none])

tzakian · 2020-08-20T18:50:33Z

language/stdlib/modules/LibraTimestamp.move

@@ -151,7 +153,7 @@ module LibraTimestamp {
    }


-    /// Gets the timestamp representing `now` in microseconds.
+    /// Gets the current time in microseconds.
    public fun now_microseconds(): u64 acquires CurrentTimeMicroseconds {


More of a meta question here: now that we have now_seconds do we want to keep now_microseconds around as part f the public API since we don't need that fine of a granularity on time?

Great question! I don't see now_microseconds used anywhere else except for AccountLimits, and I would be very much in favor of replacing that usage with now_seconds and hiding now_microseconds. Let me try to tackle this before landing.

Starting looking at this and it feels like a big enough change that it should happen in a separate PR. But definitely worth doing!

sblackshear · 2020-08-20T22:18:36Z

/land

The only purpose of LibraTransactionTimeout was to an enforce upper and lower bounds on transaction expiration time. The lower bound check is very simple to do and the upper bound check is no longer a requirement, so we can kill this module. The PR deletes LibraTransactionTimeout and moves the lower bound check into the transaction prologue. To keep the check simple, it adds a LibraTimestamp::now_seconds function that can be directly compared against the transaction expiration time (which is specified in seconds). This is a breaking change in the sense that it removes the LibraTransactionTimeout module and orphans the TTL resource, but it is not observable to clients and should be deployable via a WriteSet. Closes: diem#5674

github-actions · 2020-08-21T01:40:32Z

Cluster Test Result

all up : 890 TPS, 5083 ms latency, 6250 ms p99 latency, no expired txns

Repro cmd:

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

After diem#5674, the maximum value for transaction expiration is no longer constrained by a factor of 1_000_000, so update the comment to reflect that.

After #5674, the maximum value for transaction expiration is no longer constrained by a factor of 1_000_000, so update the comment to reflect that. Closes: #5767

sblackshear added the writeset-me label Aug 19, 2020

sblackshear requested review from tzakian, runtian-zhou, wrwg and bob-wilson August 19, 2020 17:12

bors-libra added this to In Review in bors Aug 19, 2020

diem-cla-bot bot added the cla-signed label Aug 19, 2020

sblackshear commented Aug 19, 2020

View reviewed changes

sblackshear force-pushed the cleanup_libra_time branch from d6b3725 to f7988cf Compare August 19, 2020 17:47

bob-wilson approved these changes Aug 19, 2020

View reviewed changes

sblackshear force-pushed the cleanup_libra_time branch from f7988cf to dbbe2ca Compare August 19, 2020 22:54

sblackshear requested review from riyazdf and n4ss August 20, 2020 16:18

sblackshear force-pushed the cleanup_libra_time branch from dbbe2ca to ddfd2bc Compare August 20, 2020 16:34

github-actions bot added the deps-changed label Aug 20, 2020

tzakian approved these changes Aug 20, 2020

View reviewed changes

sblackshear force-pushed the cleanup_libra_time branch from ddfd2bc to 5ae0e45 Compare August 20, 2020 22:17

bors-libra moved this from In Review to Queued in bors Aug 20, 2020

bors-libra moved this from Queued to Testing in bors Aug 21, 2020

bors-libra force-pushed the cleanup_libra_time branch from 5ae0e45 to 0aa7461 Compare August 21, 2020 01:40

bors-libra removed this from Testing in bors Aug 21, 2020

bors-libra closed this in 0aa7461 Aug 21, 2020

bors-libra merged commit 0aa7461 into diem:master Aug 21, 2020

bob-wilson mentioned this pull request Aug 25, 2020

[types] update comment about transaction expiration times #5767

Merged

sblackshear deleted the cleanup_libra_time branch September 21, 2020 14:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[breaking][libra framework] Remove LibraTransactionTimeout module #5674

[breaking][libra framework] Remove LibraTransactionTimeout module #5674

sblackshear commented Aug 19, 2020

sblackshear left a comment

bors-libra commented Aug 19, 2020

General

Commands

Options

MIRAI-bot commented Aug 19, 2020

bob-wilson left a comment

riyazdf commented Aug 20, 2020

n4ss commented Aug 20, 2020

github-actions bot commented Aug 20, 2020

tzakian Aug 20, 2020

sblackshear Aug 20, 2020

sblackshear Aug 20, 2020

sblackshear commented Aug 20, 2020

github-actions bot commented Aug 21, 2020

[breaking][libra framework] Remove LibraTransactionTimeout module #5674

[breaking][libra framework] Remove LibraTransactionTimeout module #5674

Conversation

sblackshear commented Aug 19, 2020

sblackshear left a comment

Choose a reason for hiding this comment

bors-libra commented Aug 19, 2020

General

Commands

Options

MIRAI-bot commented Aug 19, 2020

bob-wilson left a comment

Choose a reason for hiding this comment

riyazdf commented Aug 20, 2020

n4ss commented Aug 20, 2020

github-actions bot commented Aug 20, 2020

tzakian Aug 20, 2020

Choose a reason for hiding this comment

sblackshear Aug 20, 2020

Choose a reason for hiding this comment

sblackshear Aug 20, 2020

Choose a reason for hiding this comment

sblackshear commented Aug 20, 2020

github-actions bot commented Aug 21, 2020