Skip to content
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

Add missing networking RPC calls. #171

Merged
merged 1 commit into from
Feb 3, 2022
Merged

Add missing networking RPC calls. #171

merged 1 commit into from
Feb 3, 2022

Conversation

tnull
Copy link

@tnull tnull commented Mar 23, 2021 •
edited
Loading

This pull request adds the missing networking RPC calls (cf. API).

I locally tested the calls in regtest as far as I could, looks like they behave as expected.

sgeisler
sgeisler reviewed Mar 31, 2021
View reviewed changes
Copy link
Contributor

@sgeisler sgeisler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! Would be awesome if you could also add the methods to our integration testsuite so we can automatically test compatibility with supported versions of bitcoind.

@tnull
Copy link
Author

tnull commented Apr 2, 2021 •
edited
Loading

Thanks for the PR! Would be awesome if you could also add the methods to our integration testsuite so we can automatically test compatibility with supported versions of bitcoind.

I now added the integration tests as far as I could without spawning another bitcoind instance (which would for example become necessary to in order fully test the functionality of the add_node call).

On the way fixed a small bug with the list_banned call which is btw. basically undocumented in the API description.

sgeisler
sgeisler approved these changes Aug 24, 2021
View reviewed changes
Copy link
Contributor

@sgeisler sgeisler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

client/src/client.rs Outdated Show resolved Hide resolved
@sgeisler
Copy link
Contributor

I might hold off merging this till #195 is merged so we have CI again. I'd also like a second review, will beg for that on IRC once CI is running again.

@0xB10C
Copy link
Contributor

0xB10C commented Aug 25, 2021

Will review once rebased on #195 :) Please ping me if I don't.

@tnull
Copy link
Author

tnull commented Jan 20, 2022

Finally rebased! 😄 (@0xB10C)

0xB10C
0xB10C approved these changes Jan 30, 2022
View reviewed changes
Copy link
Contributor

@0xB10C 0xB10C left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

crACK 537fa9e modulo missing function documentation and #171 (comment)

client/src/client.rs Outdated Show resolved Hide resolved
0xB10C
0xB10C reviewed Jan 31, 2022
View reviewed changes
Copy link
Contributor

@0xB10C 0xB10C left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure about conventions for this repo, but you could probably squash your commits into one

client/src/client.rs Outdated Show resolved Hide resolved
client/src/client.rs Outdated Show resolved Hide resolved
@tnull tnull force-pushed the network_rpcs branch 2 times, most recently from 45442ee to 585dfdd Compare January 31, 2022 10:01
@tnull
Copy link
Author

tnull commented Jan 31, 2022

Thanks for the review! Squashed now.

@0xB10C
Copy link
Contributor

0xB10C commented Jan 31, 2022

utACK 585dfdd

@RCasatta
Copy link
Collaborator

RCasatta commented Feb 1, 2022

It looks there are some issues in the CI

@0xB10C
Copy link
Contributor

0xB10C commented Feb 1, 2022

message: "Bitcoin Core is not connected!"

Bitcoin Core needs to be connected to at least one peer when calling getblocktemplate.

@tnull
Copy link
Author

tnull commented Feb 1, 2022 •
edited
Loading

I fiddled around with re-connecting the peer, but finally found that it's much simpler and cleaner to move the network tests to the end of the integration tests, so that it doesn't matter if the peer isn't connected anymore afterwards. So it should be fixed now.

RCasatta
RCasatta approved these changes Feb 2, 2022
View reviewed changes
Copy link
Collaborator

@RCasatta RCasatta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 150953a

going to merge tomorrow if there isn't any comment from someone else (feel free to ping me if I don't)

@RCasatta RCasatta merged commit 09b0496 into rust-bitcoin:master Feb 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@0xB10C 0xB10C 0xB10C approved these changes

@RCasatta RCasatta RCasatta approved these changes

@sgeisler sgeisler sgeisler approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@tnull @sgeisler @0xB10C @RCasatta