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.

Sign up for GitHub

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

Jump to bottom

Refactor getTrustedForLedger() #4424

Merged
merged 1 commit into from
Feb 22, 2023
Merged

Refactor getTrustedForLedger() #4424

merged 1 commit into from
Feb 22, 2023

Conversation

levinwinter
Copy link
Contributor

@levinwinter levinwinter commented Feb 16, 2023 •
edited
Loading

This pull request fixes a bug that was detected automatically by the ByzzFuzz testing algorithm, as described in our accompanying paper Randomized Testing of Byzantine Fault Tolerant Algorithms. Florena Bușe (@florenabuse), Burcu Kulahcioglu Ozkan (@burcuku), and Levin N. Winter (@levinwinter) directly contributed to this work.

High Level Overview of Change

The getTrustedForLedger() method has been refactored to look for validations associated with a specific ledger ID and sequence number.

Context of Change

From now on, the method can be used to filter for specific sequence numbers which was adapted throughout the codebase.

Type of Change

  • Refactor (non-breaking change that only restructures code)

@levinwinter
Refactor getTrustedForLedger()
a3d9be7 
Look for validations associated with a specific ledger ID and sequence
number.
seelabs
seelabs approved these changes Feb 17, 2023
View reviewed changes
Copy link
Collaborator

@seelabs seelabs left a comment

Choose a reason for hiding this comment

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

👍 Code is clean and straight-forward. Thanks for the patch!

@intelliot intelliot requested a review from nbougalis February 21, 2023 17:12
mtrippled
mtrippled approved these changes Feb 22, 2023
View reviewed changes
Copy link
Collaborator

@mtrippled mtrippled left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! The testing looks sufficient.

pwang200
pwang200 approved these changes Feb 22, 2023
View reviewed changes
Copy link
Contributor

@pwang200 pwang200 left a comment

Choose a reason for hiding this comment

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

Thanks for the patch! Looks great.

@levinwinter
Copy link
Contributor Author

Thanks @mtrippled! I think the existing unit tests already cover quite some ground.

nbougalis
nbougalis approved these changes Feb 22, 2023
View reviewed changes
Copy link
Contributor

@nbougalis nbougalis left a comment

Choose a reason for hiding this comment

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

Looks fine to me. Left a few small comments, but none of them are blockers; you can address them at your discretion.

src/ripple/consensus/Validations.h Show resolved Hide resolved
src/ripple/consensus/Validations.h Show resolved Hide resolved
src/ripple/app/consensus/RCLConsensus.cpp Show resolved Hide resolved
src/ripple/app/ledger/impl/LedgerMaster.cpp Show resolved Hide resolved
src/ripple/app/ledger/impl/LedgerMaster.cpp Show resolved Hide resolved
src/ripple/app/ledger/impl/LedgerMaster.cpp Show resolved Hide resolved
@intelliot
Copy link
Collaborator

@levinwinter regarding the comments above, I'm happy either way; I will keep this PR open for now so that you have a chance to address them if you wish. Please comment here to indicate when you think that this PR is ready to merge, and I'll merge it.

@levinwinter
Copy link
Contributor Author

Thank you for the feedback, @nbougalis! Let's keep it as is for now because I think we should see how to refactor this and the surrounding logic more, which could then include your suggestions. As this is too much for this PR, I'd be happy if @intelliot could merge this.

@intelliot intelliot merged commit 2929748 into XRPLF:develop Feb 22, 2023
@levinwinter levinwinter deleted the refactor-get-trusted-for-ledger branch February 23, 2023 07:46
ximinez added a commit to ximinez/rippled that referenced this pull request Feb 23, 2023
@ximinez
Merge remote-tracking branch 'upstream/develop' into github-ci
927fb1c 
* upstream/develop:
  Fix Conan version constraint in workflows (XRPLF#4430)
  Refactor getTrustedForLedger() (XRPLF#4424)
  README: Update "Build from source" section (XRPLF#4426)
ximinez added a commit to ximinez/rippled that referenced this pull request Feb 23, 2023
@ximinez
Merge remote-tracking branch 'upstream/develop' into neverfeature
7b20b9e 
* upstream/develop:
  Fix Conan version constraint in workflows (XRPLF#4430)
  Refactor getTrustedForLedger() (XRPLF#4424)
  README: Update "Build from source" section (XRPLF#4426)
ximinez added a commit to ximinez/rippled that referenced this pull request Feb 23, 2023
@ximinez
Merge remote-tracking branch 'upstream/develop' into ledgerinit
894303d 
* upstream/develop:
  Fix Conan version constraint in workflows (XRPLF#4430)
  Refactor getTrustedForLedger() (XRPLF#4424)
  README: Update "Build from source" section (XRPLF#4426)
ximinez added a commit to ximinez/rippled that referenced this pull request Feb 23, 2023
@ximinez
Merge remote-tracking branch 'upstream/develop' into badmarker
4f23caf 
* upstream/develop:
  Fix Conan version constraint in workflows (XRPLF#4430)
  Refactor getTrustedForLedger() (XRPLF#4424)
  README: Update "Build from source" section (XRPLF#4426)
ximinez added a commit to ximinez/rippled that referenced this pull request Feb 23, 2023
@ximinez
Merge remote-tracking branch 'upstream/develop' into site-timeout
8c1e79f 
* upstream/develop:
  Fix Conan version constraint in workflows (XRPLF#4430)
  Refactor getTrustedForLedger() (XRPLF#4424)
  README: Update "Build from source" section (XRPLF#4426)
ximinez added a commit to ximinez/rippled that referenced this pull request Feb 23, 2023
@ximinez
Merge remote-tracking branch 'upstream/develop' into xrpfees-api
6a3017c 
* upstream/develop:
  Fix Conan version constraint in workflows (XRPLF#4430)
  Refactor getTrustedForLedger() (XRPLF#4424)
  README: Update "Build from source" section (XRPLF#4426)
ximinez added a commit to ximinez/rippled that referenced this pull request Feb 23, 2023
@ximinez
Merge remote-tracking branch 'upstream/develop' into gt--amm-core-fun…
3592426 
…ctionality

* upstream/develop:
  Fix Conan version constraint in workflows (XRPLF#4430)
  Refactor getTrustedForLedger() (XRPLF#4424)
  README: Update "Build from source" section (XRPLF#4426)
  README: Add a few source code starting points (XRPLF#4421)
ximinez added a commit to ximinez/rippled that referenced this pull request Feb 23, 2023
@ximinez
Merge remote-tracking branch 'upstream/develop' into badmarker_rh--nf…
1e6bd15 
…tpage

* upstream/develop:
  Fix Conan version constraint in workflows (XRPLF#4430)
  Refactor getTrustedForLedger() (XRPLF#4424)
  README: Update "Build from source" section (XRPLF#4426)
  README: Add a few source code starting points (XRPLF#4421)
dangell7 pushed a commit to Transia-RnD/rippled that referenced this pull request Mar 5, 2023
@levinwinter @dangell7
Refactor getTrustedForLedger() (XRPLF#4424)
336f03a 
Look for validations associated with a specific ledger ID and sequence
number.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mtrippled mtrippled mtrippled approved these changes

@intelliot intelliot intelliot left review comments

@seelabs seelabs seelabs approved these changes

@nbougalis nbougalis nbougalis approved these changes

@pwang200 pwang200 pwang200 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@levinwinter @intelliot @pwang200 @nbougalis @seelabs @mtrippled