Improve GetPeerInfo
#315
Improve GetPeerInfo
#315
Conversation
|
This is a follow up from #310 in which @0xB10C uncovered by bugs, thanks bro. I have had a niggling feeling that I made this mistake for a while now (add optional fields back in v17 to get tests to pass) but because this repo has so many LOC there was bucklys chance of me finding it. So massive thanks. |
tcharding
force-pushed
the
push-qornruupzows
branch
from
c903fd4 to
72decea
Compare
jamillambert
left a comment
jamillambert
left a comment
There was a problem hiding this comment.
One missing export, the rest looks good. And forgetting the exports/reexports of PeerInfo for v21 and up was my bad.
| GetMempoolDescendants, GetMempoolDescendantsVerbose, GetRpcInfo, MapMempoolEntryError, | ||
| MempoolEntry, MempoolEntryError, MempoolEntryFees, MempoolEntryFeesError, PeerInfo, | ||
| SetWalletFlag, | ||
| MempoolEntry, MempoolEntryError, MempoolEntryFees, MempoolEntryFeesError, SetWalletFlag, |
There was a problem hiding this comment.
types/src/v21/mod.rs:248 Missing PeerInfo export in network.
In rust-bitcoin#310 I got mixed up and added fields to the v17 types `GetPeerInfo` struct that do not appear in the docs. This was masked by using `Option`. And not helped by the fact that a bunch of other `Option`s are used because the docs do not match what Core returns. These bugs propagated up through the version numbers. Go over them all and sort it out. Note also that we were incorrectly re-exporting `PeerInfo` from v19 even though the `PeerInfo` struct that was actually being used is found in the version specific file along with `GetPeerInfo` - face palm. And finally, remove all the 'vXY and later' docs, this is implied by the module. These sorts of comments should only be found in `model`.
tcharding
force-pushed
the
push-qornruupzows
branch
from
72decea to
8a26cd0
Compare
The `GetPeerInfo` type wraps a vector of `PeerInfo` types. In such circumstances when we re-export the outer type super easy to either forget to re-export the inner type. Currently most of our tests only test the outer type. Add a local variable assignment that explicitly uses the inner type, there by proving all the re-exports are correct. Tested by running v21 tests _without_ the re-export bug found in review. And indeed this new test finds it.
No stress, it actually confused me for a while how it was working. See patch 2 of this PR. |
node/src/lib.rs
Outdated
| // This is kinda cool, it shows we can call any RPC method using the client. | ||
| let result: Vec<serde_json::Value> = client.call("getpeerinfo", &[]).unwrap(); | ||
| result.len() | ||
| let json = self.client.get_peer_info().expect("get_peer_info"); |
There was a problem hiding this comment.
| let json = self.client.get_peer_info().expect("get_peer_info"); | |
| let json = client.get_peer_info().expect("get_peer_info"); |
I am a bit confused with this, should it be instead inside impl Node? And there is another one in integration_test/src/lib.rs:110 impl NodeExt that is the one being used in the tests currently, that could then be removed.
There was a problem hiding this comment.
ooph the fact that this passed CI is alarming. We clearly are not running node tests in CI.
I left both this one and the one in integration tests in there because they are helper functions and currently we only provide helper functions as an extension to the client (in NodeExt) so I figured best to just leave it as is.
There was a problem hiding this comment.
Ha! the linter found it
There was a problem hiding this comment.
I left both this one and the one in integration tests in there because they are helper functions
Sounds good.
The client supports `getpeerinfo`, use it and remove the TODO.
tcharding
force-pushed
the
push-qornruupzows
branch
from
9c40e8b to
0d93ad7
Compare
0d93ad7e5ab979b2fe98cc5274e782e7f5fa74fd node: Use get_peer_info method (Tobin C. Harding)
d561e5c3344f54b17b7e9986f2ab2b96c470c3b4 Explicitly check type of GetPeerInfo vector element (Tobin C. Harding)
8a26cd0a6ea7067369b2719e8798af99696c1e8b Improve GetPeerInfo (Tobin C. Harding)
Pull request description:
In #310 I got mixed up and added fields to the v17 types `GetPeerInfo` struct that do not appear in the docs.
This was masked by using `Option`. And not helped by the fact that a bunch of other `Option`s are used because the docs do not match what Core returns.
These bugs propagated up through the version numbers. Go over them all and sort it out.
Note also that we were incorrectly re-exporting `PeerInfo` from v19 even though the `PeerInfo` struct that was actually being used is found in the version specific file along with `GetPeerInfo` - face palm.
And finally, remove all the 'vXY and later' docs, this is implied by the module. These sorts of comments should only be found in `model`.
ACKs for top commit:
jamillambert:
ACK 0d93ad7e5ab979b2fe98cc5274e782e7f5fa74fd
Tree-SHA512: a9565d056abc63a7b35d1041756f9f01e3dc98d357c3b911c531e4eb254747a28a4b4bdb8a36022d76807ec8849a5e3572688a148f6dae9d0d8716e442c2c9bf
2 participants
In #310 I got mixed up and added fields to the v17 types
GetPeerInfostruct that do not appear in the docs.This was masked by using
Option. And not helped by the fact that a bunch of otherOptions are used because the docs do not match what Core returns.These bugs propagated up through the version numbers. Go over them all and sort it out.
Note also that we were incorrectly re-exporting
PeerInfofrom v19 even though thePeerInfostruct that was actually being used is found in the version specific file along withGetPeerInfo- face palm.And finally, remove all the 'vXY and later' docs, this is implied by the module. These sorts of comments should only be found in
model.