rippled icon indicating copy to clipboard operation
rippled copied to clipboard

Add logging and improve counting of amendment votes from UNL

Open ximinez opened this issue 1 year ago • 1 comments

High Level Overview of Change

Adds debug-level logging of the amendment vote counting and decision making process. Also, tweaks the way the vote threshold is computed to only account for validators that the node has votes for, whether fresh or cached.

Context of Change

Some UNL validators were not reflecting an accurate count of the total votes for given amendments. (The problem seemed to resolve itself when the node restarted.) This made it look like the amendment did not have 80% majority, when it actually did. The situation did not resolve itself until the vote count increased again.

Because this processing was completely transparent, there was no way to tell why the votes were inaccurate, and because restarting seemed to fix the issue, it couldn't be reproduced with modified code.

Despite appearances, this issue is not security sensitive because

  1. It can only delay acceptance of an amendment
  2. It can not force an amendment to be accepted prematurely
  3. There is a workaround

While adding logging, I noticed and fixed a possible bug in the way that the total validations are counted, which affects the way the threshold is computed. Even with this fixed, though, it doesn't explain why the counts are off.

The primary purpose of this change / PR is to have logging available in case something like this ever happens again.

Type of Change

  • [X] Bug fix (non-breaking change which fixes an issue)
  • [X] Refactor (non-breaking change that only restructures code)

Test Plan

  • Set log_level for Amendments and LedgerConsensus to debug or trace.
  • After a flag ledger, search the debug log for the name and/or hash of the amendment of interest. Or just search for Amendment:.
  • There will be a lot more logs than before.

ximinez avatar Oct 31 '24 16:10 ximinez

Codecov Report

Attention: Patch coverage is 90.41096% with 7 lines in your changes missing coverage. Please review.

Project coverage is 78.2%. Comparing base (844646d) to head (ba93f58). Report is 2 commits behind head on develop.

Files with missing lines Patch % Lines
src/xrpld/app/misc/detail/AmendmentTable.cpp 90.0% 7 Missing :warning:
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##           develop   #5173     +/-   ##
=========================================
+ Coverage     77.9%   78.2%   +0.3%     
=========================================
  Files          791     790      -1     
  Lines        68006   67738    -268     
  Branches      8346    8177    -169     
=========================================
- Hits         52967   52955     -12     
+ Misses       15039   14783    -256     
Files with missing lines Coverage Δ
src/xrpld/app/consensus/RCLConsensus.cpp 65.4% <ø> (ø)
src/xrpld/app/misc/AmendmentTable.h 100.0% <100.0%> (ø)
src/xrpld/app/misc/detail/AmendmentTable.cpp 95.5% <90.0%> (-1.3%) :arrow_down:

... and 14 files with indirect coverage changes

Impacted file tree graph

codecov[bot] avatar Oct 31 '24 23:10 codecov[bot]

@vvysokikh1 I took your two suggestions. Could you confirm your approval?

ximinez avatar Feb 20 '25 17:02 ximinez