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

Remove 'new parent hash' assert #5313

Merged
merged 2 commits into from
Feb 25, 2025
Merged

Remove 'new parent hash' assert #5313

merged 2 commits into from
Feb 25, 2025

Conversation

Bronek
Copy link
Collaborator

@Bronek Bronek commented Feb 24, 2025

This assert is known to occasionally trigger, without causing errors downstream. Replace it with a log message.

High Level Overview of Change

Remove assert which is known to occasionally fail, without causing any problems.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Refactor (non-breaking change that only restructures code)
  • Performance (increase or change in throughput and/or latency)
  • Tests (you added tests for code that already exists, or your new feature included in this PR)
  • Documentation update
  • Chore (no impact to binary, e.g. .gitignore, formatting, dropping support for older tooling)
  • Release

API Impact

  • Public API: New feature (new methods and/or new fields)
  • Public API: Breaking change (in general, breaking changes should only impact the next api_version)
  • libxrpl change (any change that may affect libxrpl or dependents of libxrpl)
  • Peer protocol change (must be backward compatible or bump the peer protocol version)

This assert is known to occasionally trigger, without causing errors
downstream. Replace it with a log message.
@ximinez
Copy link
Collaborator

ximinez commented Feb 24, 2025

Thank you for taking care of this nagging issue.

Copy link

codecov bot commented Feb 24, 2025

Codecov Report

Attention: Patch coverage is 75.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 78.2%. Comparing base (ab44cc3) to head (0841d7f).
Report is 2 commits behind head on develop.

Files with missing lines Patch % Lines
src/xrpld/app/misc/detail/TxQ.cpp 75.0% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##           develop   #5313     +/-   ##
=========================================
- Coverage     78.2%   78.2%   -0.0%     
=========================================
  Files          790     790             
  Lines        67738   67739      +1     
  Branches      8177    8178      +1     
=========================================
- Hits         52962   52954      -8     
- Misses       14776   14785      +9     
Files with missing lines Coverage Δ
src/xrpld/app/misc/TxQ.h 98.2% <ø> (ø)
src/xrpld/app/misc/detail/TxQ.cpp 98.6% <75.0%> (-0.1%) ⬇️

... and 5 files with indirect coverage changes

Impacted file tree graph

@Bronek Bronek added the Ready to merge *PR author* thinks it's ready to merge. Has passed code review. Perf sign-off may still be required. label Feb 25, 2025
@bthomee bthomee merged commit 9745718 into develop Feb 25, 2025
23 of 24 checks passed
@bthomee bthomee deleted the Bronek/remove_dummy_assert branch February 25, 2025 14:14
@Bronek Bronek added the Found with Antithesis Bugs identified with the help of Anithesis continuous testing platform label Feb 25, 2025
@intelliot
Copy link
Collaborator

intelliot commented Feb 25, 2025

Out of curiosity, in what situations would this assert trigger?

When the parent ledger hash is unchanged, then the ledger has not advanced, right? Why would the same ledger be processed again?

If it is expected behavior (when?) then we could consider having the log at info. But if it does indicate something problematic, then warn is appropriate.

@legleux legleux mentioned this pull request Feb 26, 2025
5 tasks
ximinez pushed a commit that referenced this pull request Feb 28, 2025
This assert is known to occasionally trigger, without causing errors
downstream. It is replaced with a log message.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Found with Antithesis Bugs identified with the help of Anithesis continuous testing platform Ready to merge *PR author* thinks it's ready to merge. Has passed code review. Perf sign-off may still be required.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants