-
Notifications
You must be signed in to change notification settings - Fork 671
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
feat: include the node public key in /v2/info #3069
Conversation
…rted in /v2/neighbors
Codecov Report
@@ Coverage Diff @@
## develop #3069 +/- ##
===========================================
- Coverage 83.83% 83.75% -0.09%
===========================================
Files 248 248
Lines 196916 196986 +70
===========================================
- Hits 165090 164980 -110
- Misses 31826 32006 +180
Continue to review full report at Codecov.
|
Add an option to also report the miner address, but make it configurable (and off by default). |
src/net/mod.rs
Outdated
#[serde(default)] | ||
#[serde(skip_serializing_if = "Option::is_none")] | ||
pub node_public_key: Option<StacksPublicKey>, | ||
#[serde(default)] | ||
#[serde(skip_serializing_if = "Option::is_none")] | ||
pub node_public_key_hash: Option<Hash160>, |
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.
Is the goal with these fields that this struct should still be able to deserialize the older version of the /v2/info
result? If so, can you add a simple unit test that deserializes the old output. A related (but maybe harder to test) question: will old nodes ever try and fail to deserialize the new output?
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.
Is the goal with these fields that this struct should still be able to deserialize the older version of the /v2/info result?
Yes
If so, can you add a simple unit test that deserializes the old output.
Sure.
A related (but maybe harder to test) question: will old nodes ever try and fail to deserialize the new output?
Not that I'm aware of. However, I'm of the opinion that if we change the wire format of an RPC response in a backwards-incompatible way, then we need to bump the version prefix (i.e. /v3/info
). I'd rather not do that, so I made it so the code will deserialize the older /v2/info
.
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.
I added compatibility tests for the old /v2/info
structure to verify that they decode under the new structure.
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.
lgtm
let mut peer_1_stacks_node = peer_1.stacks_node.take().unwrap(); | ||
let _ = peer_1 | ||
.network | ||
.refresh_burnchain_view(&peer_1_sortdb, &peer_1_stacks_node.chainstate, false) |
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.
why did you add calls to refresh_burnchain_view
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.
The absence of this call was causing the .canonical_stacks_tip_height
values to not propagate. Adding this in fixes it.
… strings instead of JSON objects; also, test decoding into the new RPCGetInfo structure
…rk/stacks-blockchain into fix/node-public-key
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Expose the node's network public key and its Hash160 in
/v2/info
. Fixes #3046.