Skip to content

fix: discard 0 scored nodes from distribution pool - #2

Open
BjrInt wants to merge 9 commits into
mainfrom
Fix/RewardDistribution0Score
Open

fix: discard 0 scored nodes from distribution pool#2
BjrInt wants to merge 9 commits into
mainfrom
Fix/RewardDistribution0Score

Conversation

@BjrInt

@BjrInt BjrInt commented Aug 10, 2023

Copy link
Copy Markdown
Member

Nodes that have a 0% score (or sub .2 score) should not account in the total amount of rewardable nodes.

@hoh

hoh commented Aug 16, 2023

Copy link
Copy Markdown
Member

Why 0.2 and not 0 ?

@hoh hoh self-assigned this Aug 16, 2023
@MHHukiewitz

MHHukiewitz commented Mar 26, 2024

Copy link
Copy Markdown
Member

Why 0.2 and not 0 ?

Well, I guess rounding errors? @hoh

@hoh

hoh commented Mar 26, 2024

Copy link
Copy Markdown
Member

More likely due to https://docs.aleph.im/nodes/reliability/rewards/

No reward is distributed when the score is below 20% .

@hoh hoh removed their assignment May 8, 2025

@foxpatch-aleph foxpatch-aleph left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The core fix — filtering nodes with score < 0.2 from the distribution pool — is correct and aligns well with the existing compute_score_multiplier function. The _prepare_crn_url URL-deduplication feature is a reasonable addition, but it contains a genuine bug: urlparse("").hostname returns None, causing false hostname collisions between any two nodes that have an empty or invalid address. This could silently blank the address field of legitimate CRN registrations. The staking-related behavioral change (stakers on sub-0.2 nodes also get excluded from the reward pool) is a meaningful side-effect that deserves a comment. New logic is also missing test coverage.

src/aleph_nodestatus/status.py (line 126): Bug: None hostname causes false collisions. urlparse("").hostname returns None. If any existing resource node has an empty or invalid address (which is the default on creation), node_hostname will also be None, and the comparison None == None will be True, causing the function to return '' for every subsequent CRN that also lacks an address. This will silently blank the address of legitimate new nodes.

Fix by guarding against None:

node_hostname = urlparse(address).hostname
if not node_hostname:
    return address  # can't deduplicate without a valid hostname

src/aleph_nodestatus/status.py (line 121): The method is async but contains no await expressions — it can be a plain def. This is a minor correctness point (nothing breaks, but it misleads readers into expecting I/O).

src/aleph_nodestatus/status.py (line 480): Line exceeds typical length limits. Consider extracting the details.get(…) call to a local variable for readability:

new_address = details.get(field, node.get(field, ""))
node[field] = await self._prepare_crn_url(node, address=new_address)

src/aleph_nodestatus/distribution.py (line 148): The score filter correctly mirrors compute_score_multiplier (which returns 0 for score < 0.2). However, filtering these nodes out of active_nodes also excludes their stakers from total_staked (line 160), meaning stakers backing a sub-0.2 node receive zero rewards while their stake remains locked. This is a non-obvious side-effect worth documenting with a comment, e.g.:

# Nodes below 0.2 score are ineligible; their stakers also receive no rewards.

src/aleph_nodestatus/status.py (line 121): No tests cover _prepare_crn_url: the hostname-deduplication logic (normal case, collision case, empty-address edge case) should have unit tests before this ships.

src/aleph_nodestatus/distribution.py (line 148): The score >= 0.2 filter in process_distribution has no unit test. Given that this is the primary stated goal of the PR, a test asserting that nodes with score < 0.2 are excluded from both active_nodes and the reward calculation (and that remaining nodes receive the correct per-node share) would protect this invariant.

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.

6 participants