ProofRecorder: Implement transactional support #13769

bkchr · 2023-03-30T14:14:08Z

This implements transactional support for ProofRecorder. A transaction can be started with start_transaction and then later commited or rolled back. This is important for block producers to not include data from transactions that didn't make it to the block, because they e.g. panicked or similar.

Closes: #10222

dmitry-markin

I'm not familiar with this part of the codebase enough to approve, but left some comments.

primitives/trie/src/recorder.rs

Co-authored-by: Dmitry Markin <[email protected]>

primitives/trie/src/recorder.rs

michalkucharczyk · 2023-03-31T15:28:10Z

primitives/trie/src/recorder.rs

+
+	/// Rollback the latest transaction.
+	///
+	/// If there isn't any transaction, nothing gonna happen.


Maybe a log error message would be reasonable? Otherwise we can silently skip API misuses.

I thought of returning an error as well as we do it in OverlayedChanges. However, I'm not really sure that there is any benefit in it? Even if some calls all the time rollback, nothing breaks? No assumption is broken or similar.

I was thinking about getting some information if higher level are not calling start and rollback/commit in pairs (e.g. due to some error in logic). W/o error message this case will be silently skipped (maybe making future debugging more tricky).

If this is a conscious design decision that rollback/commit can be called w/o matching start than it is also OK.

Logging error could be some compromise between silently ignoring this case and returning the error.

But maybe my reasoning is too defensive here.

michalkucharczyk · 2023-03-31T15:29:21Z

primitives/trie/src/recorder.rs

+	pub fn start_transaction(&self) {
+		let mut inner = self.inner.lock();
+		inner.accessed_nodes_transactions.push(Default::default());
+		inner.recorded_keys_transactions.push(Default::default());


There is an assumption that lenght of *_transactions are equal. How about adding assertion here?

I mean both walk in lockstep, but there is no assumption that both have the same length? We never access anything from one to the other assuming that the same index exists there as well.

I meant silent assumption - at the end both vecs should have the length of number of transactions, it should never happen that one contains more then other. Assert would be more expressive way to indicate this bound. It would also help if some modifies the code (e.g. return early from rollback/commit_transaction).

Anyway, feel free to ignore it.

skunert · 2023-04-03T07:33:01Z

test-utils/runtime/src/system.rs

+
+			next_key = next;
+		} else {
+			if panic_at_end {


Do we need this condition?
When execute ReadAndPanic(10) we expect to do 10 reads and then panic right? If we are only able to do 5 reads it looks from the outside as if everything is ok, but something unexpected happened.

Yes we need this.

The docs explicitly mention this:

/// Read X times from the state some data and then panic! /// /// Returns `Ok` if it didn't read anything.

The point is that you expect it to fail and if it doesn't fail, you know that something didn't work as expected.

primitives/trie/src/recorder.rs

Co-authored-by: Sebastian Kunert <[email protected]>

bkchr · 2023-04-03T20:27:15Z

bot fmt

command-bot · 2023-04-03T20:27:20Z

@bkchr https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2636246 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh". Check out https://gitlab.parity.io/parity/mirrors/substrate/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 46-2d311f98-9a7d-4a2c-aac2-43d00e1ad909 to cancel this command or bot cancel to cancel all commands in this pull request.

command-bot · 2023-04-03T20:28:34Z

@bkchr Command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh" has finished. Result: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2636246 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2636246/artifacts/download.

bkchr · 2023-04-03T21:48:51Z

bot rebase

…-transactions

paritytech-processbot · 2023-04-03T21:48:59Z

Rebased

bkchr · 2023-04-03T21:49:50Z

bot merge

paritytech-processbot · 2023-04-03T21:49:58Z

Waiting for commit status.

paritytech-processbot · 2023-04-03T21:52:28Z

Merge cancelled due to error. Error: Statuses failed for f7e8b48

primitives/trie/src/recorder.rs

Co-authored-by: Anton <[email protected]>

bkchr · 2023-04-05T14:15:24Z

bot merge

paritytech-processbot · 2023-04-05T14:15:32Z

Waiting for commit status.

* TrieRecorder: Start adding support for transactions * Adds `transactions` functions and some test * More tests * Docs * Ensure that we rollback failed transactions in the storage proof * FMT * Update primitives/trie/src/recorder.rs Co-authored-by: Dmitry Markin <[email protected]> * Review comments * Update primitives/trie/src/recorder.rs Co-authored-by: Sebastian Kunert <[email protected]> * ".git/.scripts/commands/fmt/fmt.sh" * For the holy clippy! * Update primitives/trie/src/recorder.rs Co-authored-by: Anton <[email protected]> --------- Co-authored-by: Dmitry Markin <[email protected]> Co-authored-by: Sebastian Kunert <[email protected]> Co-authored-by: command-bot <> Co-authored-by: Anton <[email protected]>

Polkadot-Forum · 2023-05-02T08:44:14Z

This pull request has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/april-updates-for-substrate-and-polkadot-devs/2764/1

* TrieRecorder: Start adding support for transactions * Adds `transactions` functions and some test * More tests * Docs * Ensure that we rollback failed transactions in the storage proof * FMT * Update primitives/trie/src/recorder.rs Co-authored-by: Dmitry Markin <[email protected]> * Review comments * Update primitives/trie/src/recorder.rs Co-authored-by: Sebastian Kunert <[email protected]> * ".git/.scripts/commands/fmt/fmt.sh" * For the holy clippy! * Update primitives/trie/src/recorder.rs Co-authored-by: Anton <[email protected]> --------- Co-authored-by: Dmitry Markin <[email protected]> Co-authored-by: Sebastian Kunert <[email protected]> Co-authored-by: command-bot <> Co-authored-by: Anton <[email protected]>

bkchr added 5 commits March 28, 2023 16:53

TrieRecorder: Start adding support for transactions

f3f8bf4

Adds transactions functions and some test

0e9f8ad

More tests

5b64a40

Docs

53f5bef

Ensure that we rollback failed transactions in the storage proof

93b861f

FMT

5b36ad0

bkchr requested a review from a team March 30, 2023 14:19

dmitry-markin reviewed Mar 31, 2023

View reviewed changes

primitives/trie/src/recorder.rs Outdated Show resolved Hide resolved

primitives/trie/src/recorder.rs Outdated Show resolved Hide resolved

dmitry-markin requested a review from a team March 31, 2023 08:10

Update primitives/trie/src/recorder.rs

7910245

Co-authored-by: Dmitry Markin <[email protected]>

michalkucharczyk approved these changes Mar 31, 2023

View reviewed changes

michalkucharczyk requested a review from a team March 31, 2023 15:48

Review comments

7824280

skunert reviewed Apr 3, 2023

View reviewed changes

Update primitives/trie/src/recorder.rs

622a633

Co-authored-by: Sebastian Kunert <[email protected]>

bkchr requested a review from skunert April 3, 2023 20:27

".git/.scripts/commands/fmt/fmt.sh"

99f41d2

skunert approved these changes Apr 3, 2023

View reviewed changes

Merge remote-tracking branch 'origin/master' into bkchr-trie-recorder…

f7e8b48

…-transactions

melekes reviewed Apr 5, 2023

View reviewed changes

primitives/trie/src/recorder.rs Show resolved Hide resolved

primitives/trie/src/recorder.rs Outdated Show resolved Hide resolved

bkchr and others added 2 commits April 5, 2023 16:07

For the holy clippy!

3bc9cbe

Update primitives/trie/src/recorder.rs

a346fca

Co-authored-by: Anton <[email protected]>

paritytech-processbot bot merged commit 5601965 into master Apr 5, 2023

paritytech-processbot bot deleted the bkchr-trie-recorder-transactions branch April 5, 2023 14:43

github-actions bot mentioned this pull request Jun 15, 2023

Update substrate/polkadot/cumulus from v0.9.40 to v0.9.43 moonbeam-foundation/moonbeam#2354

Closed

nazar-pc mentioned this pull request Jun 16, 2023

Upgrade Substrate (with core-evm-relay removed) autonomys/subspace#1549

Merged

1 task

kacperzuk-neti mentioned this pull request Jun 23, 2023

Polkadot v0.9.43 liberland/liberland_substrate#295

Merged

15 tasks

CanonicalJP mentioned this pull request Oct 3, 2023

PR#13769 ProofRecorder: Implement transactional support paritytech/polkadot-sdk#1776

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ProofRecorder: Implement transactional support #13769

ProofRecorder: Implement transactional support #13769

bkchr commented Mar 30, 2023

dmitry-markin left a comment

michalkucharczyk Mar 31, 2023

bkchr Mar 31, 2023

michalkucharczyk Apr 1, 2023

bkchr Apr 1, 2023

michalkucharczyk Mar 31, 2023

bkchr Mar 31, 2023

michalkucharczyk Apr 1, 2023

bkchr Apr 1, 2023

skunert Apr 3, 2023

bkchr Apr 3, 2023

bkchr commented Apr 3, 2023

command-bot bot commented Apr 3, 2023 •

edited

Loading

command-bot bot commented Apr 3, 2023

bkchr commented Apr 3, 2023

paritytech-processbot bot commented Apr 3, 2023

bkchr commented Apr 3, 2023

paritytech-processbot bot commented Apr 3, 2023

paritytech-processbot bot commented Apr 3, 2023

bkchr commented Apr 5, 2023

paritytech-processbot bot commented Apr 5, 2023

Polkadot-Forum commented May 2, 2023

ProofRecorder: Implement transactional support #13769

ProofRecorder: Implement transactional support #13769

Conversation

bkchr commented Mar 30, 2023

dmitry-markin 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkchr commented Apr 3, 2023

command-bot bot commented Apr 3, 2023 • edited Loading

command-bot bot commented Apr 3, 2023

bkchr commented Apr 3, 2023

paritytech-processbot bot commented Apr 3, 2023

bkchr commented Apr 3, 2023

paritytech-processbot bot commented Apr 3, 2023

paritytech-processbot bot commented Apr 3, 2023

bkchr commented Apr 5, 2023

paritytech-processbot bot commented Apr 5, 2023

Polkadot-Forum commented May 2, 2023

command-bot bot commented Apr 3, 2023 •

edited

Loading