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

Add logging and improve counting of amendment votes from UNL #5173

Open
wants to merge 9 commits into
base: develop
Choose a base branch
from

Conversation

ximinez
Copy link
Collaborator

@ximinez ximinez commented Oct 31, 2024

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

  • Bug fix (non-breaking change which fixes an issue)
  • 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 ximinez marked this pull request as ready for review October 31, 2024 20:30
@ximinez ximinez requested review from vlntb and removed request for gregtatcam October 31, 2024 20:32
@ximinez ximinez assigned vlntb and unassigned gregtatcam Oct 31, 2024
* upstream/develop:
  Add hubs.xrpkuwait.com to bootstrap (5169)
Copy link

codecov bot commented Oct 31, 2024

Codecov Report

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

Project coverage is 77.9%. Comparing base (f64cf91) to head (72e0dbb).

Files with missing lines Patch % Lines
src/xrpld/app/misc/detail/AmendmentTable.cpp 89.7% 7 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           develop   #5173   +/-   ##
=======================================
  Coverage     77.9%   77.9%           
=======================================
  Files          784     784           
  Lines        66681   66732   +51     
  Branches      8162    8144   -18     
=======================================
+ Hits         51950   51996   +46     
- Misses       14731   14736    +5     
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% <89.7%> (-1.4%) ⬇️

... and 14 files with indirect coverage changes

Impacted file tree graph

* upstream/develop:
  Add AMMClawback Transaction (XLS-0073d) (5142)
* upstream/develop:
  Set version to 2.3.0-rc1
  Replace Uint192 with Hash192 in server_definitions response (5177)
  Fix potential deadlock (5124)
  Introduce Credentials support (XLS-70d): (5103)
  Fix token comparison in Payment (5172)
  Add fixAMMv1_2 amendment (5176)
* upstream/develop:
  fix: include `index` in `server_definitions` RPC (5190)
  Fix ledger_entry crash on invalid credentials request (5189)
* upstream/develop:
  Set version to 2.3.0-rc2
* upstream/develop:
  Set version to 2.3.0
  refactor(AMMClawback): move tfClawTwoAssets check (5201)
  Add a new serialized type: STNumber (5121)
  fix: check for valid ammID field in amm_info RPC (5188)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants