Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
R4R: Enforce block maximum gas limit in DeliverTx #2795
R4R: Enforce block maximum gas limit in DeliverTx #2795
Changes from 22 commits
2f73cf4
956d351
ebaa394
3bf67b6
8069b2b
2446830
eead278
7e6fcc0
68e3b9a
0d4dd87
5249064
7be5179
90217e2
2a594fe
4818e67
56dc236
d911565
10bdf8f
4afd53d
bd982b1
70e60c2
6fd3132
972377c
b4b61b8
abed373
2d3e1af
56fa7dc
0861112
ce10ef2
5792e1d
819af35
0da2472
f12ac43
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably good to indicate that its local spam prevention, not consensus
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't want to use a codec here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure @jaekwon
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
switched to codec - Jae can switch back if there is a good reason. Otherwise yeah staying consistent makes sense
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be proto because abci.ConsensusParams are protobuf messages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this further alludes to the statement above. I don't see the direct relationship between these params and the ones used via the context. I think at the very least the godoc should allude to what these consensus params are for.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question. The types/Context consensus params were never used. We don't need them in the context for now, and even if we need them we'd want to modify them via a keeper, so for now will delete types/Context.ConsensusParams.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if this panics again? Won't
DeliverTx
panic, whereas we want to just return an error?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would specifically panic again?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
ConsumeGas
call in the recover handler here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this panics it will be caught by the recover (just above this comment) - this comment simply notes that even if this consume gas panics, it will still affect the state of the BlockGasMeter which is important for ignoring future transactions... updating this comment to reflect this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it won't. I've added a demonstrative failing testcase: 2d3e1af.
Run
go test ./baseapp/... -v
, it panics with: