rpc: implement getaddednodeinfo#915
Conversation
Wire through the full RPC stack to expose information about manually added peers (via addnode add), matching Bitcoin Core's getaddednodeinfo behavior. Layers touched: - UserRequest/NodeResponse enums and NodeInterface method - Handler in perform_user_request with filter_map logic - FlorestaRPC trait + JsonRPCClient impl - Server dispatcher + RpcImpl method - CLI command with doc binding - RPC doc, integration test, and test framework client method
csgui
left a comment
csgui
left a comment
There was a problem hiding this comment.
Hey @Micah-Shallom !
Thanks, but can you please clarify the extent of LLM usage in this PR?
|
Hi @csgui thank for the heads up ....i used AI to help setup the tests and doc template....the RPC implementation itself follows the same pattern as the existing endpoints i have been working on lately. |
|
@Micah-Shallom I’m asking because this PR comes across as somewhat mechanical. As reviewers, we value understanding the contributor’s workflow, how you arrived at the proposed solution, including any iterations or mistakes along the way. Ideally, commits should read like a conversation, helping us follow the reasoning behind the changes. Our goal is to foster technical discussion around both the project and its implementation. Every PR requires careful review, and feedback on AI-generated code can end up becoming just another input to the AI, which may not be the most effective use of everyone's time. Ultimately, we aim for a process that benefits both sides: reviewers learn from contributors and contributors deepen their understanding of the project. In that sense, a PR review should be a moment of knowledge sharing. Because of this, PRs that appear heavily AI-generated tend to be a lower priority for the team and may not be accepted into the main codebase. Our main goal is to encourage knowledge sharing, discussion and clear technical storytelling about how a problem is solved, making the process a win-win for everyone involved. |
|
Thanks for your feedback @csgui .... I'll keep this in mind going forward |
|
Issue #906 is already closed. If there isn't a specific open issue related to what the PR addresses, please open one and reference it here. This is especially useful for RPC work. |
Tests should be done manually, especially. This is where most of the thinking should happen, creating a safety net for the changes you're proposing to the codebase. Also, AI isn't very good at identifying subtle corner cases (at least not so far). |
| /// Information about a manually added peer, as returned by `getaddednodeinfo`. | ||
| pub struct AddedNodeInfoItem { | ||
| /// The address of the added node in `ip:port` format. | ||
| pub addednode: String, |
There was a problem hiding this comment.
floresta’s RPCs need to be as close as possible to the core standard, so here it’s just the ip from core without the port
https://bitcoincore.org/en/doc/30.0.0/rpc/network/getaddednodeinfo/
There was a problem hiding this comment.
thanks...this will be addressed on PR #916
|
|
||
| ## Notes | ||
|
|
||
| - Only nodes added via `addnode add` are listed. Nodes connected with `addnode onetry` are not included. |
There was a problem hiding this comment.
I think this information is wrong based on the code I saw. It would be useful to demonstrate this in the Python test, since this is a rule for this RPC that must be followed
There was a problem hiding this comment.
@moisesPompilio thanks for your feedbacks... the way getaddednodeinfo was implemented here was such that it reads from added_pairs ...thats the reason onetry peers won't appear...cus handle_addnode_onetry_peer never adds to added_peers
@jaoleal opened a PR with this implementation there and we decided to use that...your reviews there too will be highly appreciated...
|
Sorry @Micah-Shallom but complementing on #915 (comment) before this goes any further. Can we continue the implementation of this rpc on #916 ? I really could use a PR review there |
|
okay @jaoleal your PR looks solid..lets continue the work there...i will do some more reviews on it...thanks |
4 participants
Summary
getaddednodeinfoRPC endpoint as listed in [RPCSAGA] Network RPCs, what we need ? #906addnodeRPC commandnodeparameter to filter results by a specific addressgetaddednodeinfoTest plan
getaddednodeinfo.py— starts florestad + bitcoind, adds peer viaaddnode add, asserts correct response structure and filtering by IPSuccessful test run
RPC doc