-
Notifications
You must be signed in to change notification settings - Fork 985
use error code 3 for reverts #9365
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
Merged
+5
−3
Merged
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
b611739
use error code 3 for reverts
macfarla c426a49
Merge branch 'main' of github.com:hyperledger/besu into revert-error-…
macfarla 7eb87cf
Merge branch 'main' of github.com:hyperledger/besu into revert-error-…
macfarla 2964bf1
update tests for revert error code 3
macfarla b6cee28
changelog
macfarla 31c3878
merge
macfarla 914854e
Merge branch 'main' into revert-error-code
macfarla 9bc8267
Merge branch 'main' into revert-error-code
macfarla 44cb8d9
Merge branch 'main' into revert-error-code
macfarla File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
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.
This feels like quite a big breaking change to just release without warning, especially since it'll end up in the fusaka release. Does this warrant 3 months notice?
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.
Ambivalent here. If geth and erigon, both of which are used for block explorers and rpc instances, have used this since 2022... how much would it really break?
Maybe private chains or alt-L1s?
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.
That lessens the concern somewhat.
Are you suggesting that noone is using eth_call/eth_estimateGas with besu on mainnet? I don't know the answer to that, I just thought it likely someone is.
For sure.
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.
yeah this was kind of my thinking. leaving it until after the fusaka release is nbd. but yeah this is bringing besu in line with other ELs so it's probably only really a breaking change if you're only using besu - so private chains.
maybe there is a bigger discussion to have around how we communicate changes to our RPCs
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 believe historically the default for breaking RPC is 3 months notice. Previously think it was tied to quarterly releases, now think we agreed "upcoming breaking changes" is sufficient.
I'm not sure whether breaking RPCs for private networks should be treated differently to public networks.
I don't feel too strongly if you are convinced the impact is low, just wanted to flag it.
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.
since this change brings besu in line with other ELs, think it's ok to flag in the changelog in the same release. No advance warning needed.