Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

runtime: Verify read/write sets during transaction execution #2081

Closed
wants to merge 1 commit into from

Conversation

kostko
Copy link
Member

@kostko kostko commented Sep 12, 2019

Fixes #2012.
Based on #2054.

TODO

  • Add/update tests.

@kostko kostko added the c:breaking/consensus Category: breaking consensus changes label Sep 12, 2019
@kostko kostko force-pushed the kostko/feature/rt-verify-rw-set branch 3 times, most recently from be97f66 to 5595d43 Compare September 12, 2019 14:50
@codecov
Copy link

codecov bot commented Sep 12, 2019

Codecov Report

Merging #2081 into kostko/feature/storage-rs-v2 will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@                       Coverage Diff                        @@
##           kostko/feature/storage-rs-v2    #2081      +/-   ##
================================================================
- Coverage                          53.5%   53.48%   -0.02%     
================================================================
  Files                               266      266              
  Lines                             25687    25687              
================================================================
- Hits                              13743    13739       -4     
- Misses                            10479    10482       +3     
- Partials                           1465     1466       +1
Impacted Files Coverage Δ
go/scheduler/tendermint/tendermint.go 64.28% <0%> (-4.09%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bdb1b4d...5595d43. Read the comment docs.

@codecov
Copy link

codecov bot commented Sep 12, 2019

Codecov Report

Merging #2081 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2081   +/-   ##
=======================================
  Coverage   52.73%   52.73%           
=======================================
  Files         267      267           
  Lines       26331    26331           
=======================================
  Hits        13886    13886           
  Misses      10984    10984           
  Partials     1461     1461

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e6cb892...ecd5707. Read the comment docs.

@kostko kostko force-pushed the kostko/feature/rt-verify-rw-set branch from 5595d43 to fe7b0d8 Compare September 12, 2019 15:44
@kostko kostko changed the base branch from kostko/feature/storage-rs-v2 to master September 12, 2019 15:53
@kostko kostko force-pushed the kostko/feature/rt-verify-rw-set branch 3 times, most recently from 1940693 to ca83f89 Compare September 13, 2019 11:01
@kostko kostko marked this pull request as ready for review September 13, 2019 11:02
@kostko kostko force-pushed the kostko/feature/rt-verify-rw-set branch from ca83f89 to b6416c4 Compare September 13, 2019 11:49
Copy link
Contributor

@Yawning Yawning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd feel better about approving this if I understood the storage code in more depth.

@kostko kostko force-pushed the kostko/feature/rt-verify-rw-set branch from b6416c4 to ecd5707 Compare September 16, 2019 07:39
@kostko
Copy link
Member Author

kostko commented Sep 16, 2019

I did some more thinking on this and I think this also needs support for some runtime-specific integration (hooks for handling reverts and accessing the read/write set so that its hash can be included with the signed transaction).

@kostko
Copy link
Member Author

kostko commented Sep 16, 2019

My concerns are the following (points added to the agenda for the next design meeting):

  • What should happen when the specified read/write set is invalid? The current implementation will just revert all the storage side-effects of the whole transaction which is safe but may be a DoS vector as the revert doesn’t cost any gas.
    • In case it needs to do gas accounting, this could also violate the read/write set or should that be excluded?
    • Excluding it would only be possible if we could make sure that the gas accounting operations can't introduce conflicts (related to Runtime gas disbursement #2071 and special atomic storage operations).
  • Read/write sets probably need to be authenticated by the caller (transaction signer) if they can cost the caller gas.

@kostko kostko added the s:blocked Status: blocked on other work label Sep 17, 2019
@kostko
Copy link
Member Author

kostko commented Sep 26, 2019

Deprioritized.

@kostko kostko closed this Sep 26, 2019
@kostko kostko deleted the kostko/feature/rt-verify-rw-set branch April 27, 2021 11:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c:breaking/consensus Category: breaking consensus changes s:blocked Status: blocked on other work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Runtime should verify read/write sets
2 participants