-
Notifications
You must be signed in to change notification settings - Fork 43
Add hidden RPC getrawaddrman
#439
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
base: master
Are you sure you want to change the base?
Conversation
|
Ok so I learned that only the public RPCs should be added to the rustdoc table in the |
Add support for the hidden getrawaddrman RPC (v26) that returns raw address manager table entries. This RPC is marked as experimental and hidden from the API docs by default. The mapped_as and source_mapped_as fields are Optional<u32> following the established codebase pattern for version-specific fields. These fields were added in Bitcoin Core v28 and will deserialize as None for earlier versions or when -asmap is not configured. Includes integration test.
bd26946 to
bafede8
Compare
| /// The key in the parent map is formatted as "bucket/position" indicating the | ||
| /// location in the relevant address manager table. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This key is a bit weird (not in your implementation, but in the Bitcoin Core implement). IIRC, it was done this way, as the key should be seen as Bitcoin Core implementation specific. However, RPC users might want to consume and act on it. I guess probably best to write a parser for it in downstream apps and not here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, so I'm not changing anything, the key is just a String like "255/50" as you get from the RPC directly.
| node.client.add_peer_address(peer_address, peer_port).expect("addpeeraddress"); | ||
|
|
||
| let json: GetRawAddrman = node.client.get_raw_addrman().expect("getrawaddrman"); | ||
|
|
||
| let entry = json | ||
| .new | ||
| .values() | ||
| .find(|e| e.address == peer_address && e.port == peer_port) | ||
| .expect("added peer should appear in the 'new' table"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addpeeraddress supports an additional argument for adding things to the tried table. However that wasn't implemented in #334.
I think for this test it's fine to just test the new table.
| .find(|e| e.address == peer_address && e.port == peer_port) | ||
| .expect("added peer should appear in the 'new' table"); | ||
|
|
||
| assert_eq!(entry.network, "ipv4"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| assert_eq!(entry.network, "ipv4"); | |
| assert_eq!(entry.network, "ipv4"); | |
| assert!(entry.services > 0); | |
| assert!(entry.time > 0); |
I don't recall what source is set to when using addpeeraddress, but maybe that can be checked as well.
| assert_eq!(entry.network, "ipv4"); | ||
|
|
||
| // mapped_as field added in v28, only present with -asmap config. | ||
| assert!(entry.mapped_as.is_none(), "mapped_as requires -asmap config"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| assert!(entry.mapped_as.is_none(), "mapped_as requires -asmap config"); | |
| assert!(entry.mapped_as.is_none(), "mapped_as requires -asmap config"); | |
| assert!(entry.source_mapped_as.is_none(), "mapped_as requires -asmap config"); |
This might break once/if asmap get's included and enabled by default. But that's probably a few versions out.
|
Looks good to me! https://github.com/0xB10C/addrman-observer still uses a custom rust-bitcoincore-rpc |
I was going to propagate the change once all good here, but go for it! |
| /// Field order matches Bitcoin Core's RPC response definition. | ||
| #[derive(Clone, Debug, PartialEq, Deserialize, Serialize)] | ||
| #[cfg_attr(feature = "serde-deny-unknown-fields", serde(deny_unknown_fields))] | ||
| pub struct RawAddrmanEntry { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| pub struct RawAddrmanEntry { | |
| pub struct RawAddrManEntry { |
And same everywhere else that rawaddrman is converted to a Rust type.
| /// Mapped AS (Autonomous System) number at the end of the BGP route to the peer. | ||
| /// Only present if the -asmap config option is set. | ||
| /// Added in Bitcoin Core v28. | ||
| pub mapped_as: Option<u32>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this and props for getting as far as you did in this convoluted repo. The version specific types are designed to be exactly correct for the version i.e., v26::RawAddrManEntry should be exactly the shape returned by Core v26. This means no optional fields. Rather anytime a new field is added a new version of the struct is added, so we should have a v28::RawAddrMan (and v28::RawAddrManEntry).
(If there was an associated struct in model, where there is not in this case, it would use optional fields because the model types are version in-specific.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FTR looks like you have the client stuff correct, because the v28 changes do not effect the client so the v26 client macro works just fine. I mentioned that this repo was convoluted, right?
|
Everything else looks good. Reviewed: bafede8 |
Add support for the hidden
getrawaddrmanRPC that returns raw address manager table entries (added v26). This RPC is marked as experimental and hidden from the API docs by default.The
mapped_asandsource_mapped_asfields areOption<u32>following the established pattern for version-specific fields (nointo.rsrequired because these are additions rather than removals or changed types). These fields were added in Bitcoin Core v28 and will deserialise asNonefor earlier versions or when-asmapis not configured.Note: will have conflicts with #435 and #433, happy to rebase after either merges