8000 v2.2: consensus: remove early return in OC loop to fix RPC notifications (backport of #6645) by mergify[bot] · Pull Request #6680 · anza-xyz/agave · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

v2.2: consensus: remove early return in OC loop to fix RPC notifications (backport of #6645) #6680

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
Open
wants to merge 1 commit into
base: v2.2
Choose a base branch
from
Open

v2.2: consensus: remove early return in OC loop to fix RPC notifications (backport of #6645) #6680

wants to merge 1 commit into from
+1 −1

Conversation

mergify[bot]
Copy link
@mergify mergify bot commented Jun 20, 2025

Problem

#3136 changed the OC processing loop to aggregate every slot in the vote tower instead of only considering the last slot. It kept the early exit logic in case we processed a slot we had seen before.

This ends up with us always exiting early since the tower is incremental e.g:
We vote
Tower: [1, 0]
Tower: [5, 4, 3, 2, 1, 0]
The code will aggregate 5, 4, 3, and 2 and early exit once we get to 1.

This is fine for the OC and propagation check aggregation, but this early exit causes us to omit the RPC notification send logic at the very end of the processing.

Summary of Changes

Instead of early exiting, break out of the loop so that the RPC notification is still sent.

Original report

https://discord.com/channels/428295358100013066/489504501749907468/1384735277578453022

I believe the reason this sometimes work is because we don't perform this early exit for gossip votes. I'm guessing that solana-test-validator could rarely end up consuming its own gossip vote before its replay vote.

Testing

With this fix, we receive notifications again from solana-test-validator --rpc-pubsub-enable-vote-subscription:

$ wscat -c ws://127.0.0.1:8900 -x '{ "jsonrpc": "2.0", "id": 1, "method": "voteSubscribe" }' -w 5

{"jsonrpc":"2.0","result":0,"id":1}
{"jsonrpc":"2.0","method":"voteNotification","params":{"result":{"votePubkey":"4teHS2G2eNtm71m6M2TJanDJSwJMK7dhbzhDVtcu2WiN","slots":[187,188,189,190,191,192,193,194,195,196,197,198,199,200,201,202,203,204,206,207,208,209,210,211,212,213,214,215,216,217,218],"hash":"6byY4pwX1PUbUtwrzZmGugBppocarpok57BnWc5faYnQ","timestamp":1750272589,"signature":"jsScMFaEX9mKbk8rVrSZ4UjXj8umooLWfHFvQ6txu2WRgtmUNjCSG6jfgmRu7Ry2StmYFmmxNYFZQHwGABHJTpS"},"subscription":0}}
{"jsonrpc":"2.0","method":"voteNotification","params":{"result":{"votePubkey":"4teHS2G2eNtm71m6M2TJanDJSwJMK7dhbzhDVtcu2WiN","slots":[189,190,191,192,193,194,195,196,197,198,199,200,201,202,203,204,206,207,208,209,210,211,212,213,214,215,216,217,218,219,220],"hash":"8yfjjQNgGh1UrQFsNfhfiMMFaeF9JAaJoAH3FpqSoSzX","timestamp":1750272589,"signature":"3ExEQWxRknj6y6KZp3Ggo193AYFVKaosbquacHxwa5mKHqxRQ7TkSUzFRfMhABY1Zkwc3jNbJizky7VXj6o8Aa7k"},"subscription":0}}
{"jsonrpc":"2.0","method":"voteNotification","params":{"result":{"votePubkey":"4teHS2G2eNtm71m6M2TJanDJSwJMK7dhbzhDVtcu2WiN","slots":[191,192,193,194,195,196,197,198,199,200,201,202,203,204,206,207,208,209,210,211,212,213,214,215,216,217,218,219,220,221,222],"hash":"HQNXcfqkQicY9N2vfuUvuu6QYMvXUK9aLgqPEjnsbXiS","timestamp":1750272590,"signature":"2Y2LqYa4xEDc6MDfCG9izH2kTb9uWtY5gvgXKJva63wqfuLhH8Uc3SgwHc6zuRhNMk7t3b4U3ruqg3NnSum8HSjN"},"subscription":0}}
```<hr>This is an automatic backport of pull request #6645 done by [Mergify](https://mergify.com).

@mergify mergify bot requested a review from a team as a code owner June 20, 2025 21:57
@steveluscher
Copy link

I requested a backport of this; the vote_subscribe RPC subscription is essentially broken right now, which breaks both consumers and tests inside the JavaScript client library.

@steveluscher steveluscher mentioned this pull request Jun 20, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@AshwinSekar AshwinSekar

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@steveluscher @AshwinSekar
0