feat(providers): add a subset of admin namespace #1880

Rjected · 2022-11-22T05:13:12Z

Motivation

Geth includes an admin namespace which is especially useful for querying its p2p details and prompting the node to add or remove peers.

Solution

Add NodeInfo for the admin_nodeInfo return type
Add PeerInfo for the admin_peers return type
Add the following admin namespace RPCs to Middleware, implementing them on Provider<P>:
- admin_nodeInfo
- admin_peers
- admin_addPeer
- admin_addTrustedPeer
- admin_removePeer
- admin_removeTrustedPeer

This also modifies the Geth and GethInstance structs to support custom chain ids and running without the --dev flag. --dev-specific configuration options, and options that are incompatible with --dev are kept mutually exclusive with the PrivateNetOptions enum. Unless a dev-incompatible option is set, --dev mode will remain the default. An option for disabling discovery is also added.

This also adds a simple test using the new Geth options to set a few custom fields that are visible in the admin_nodeInfo output.

Misc change:

expose from_int_or_hex as a general utility so it can be used for admin_nodeInfo response deserialization.

TODO:

cover other RPCs - test for adding and removing peers, as well as listing peers.
- the test added does the following:
  - connect two geth peers
  - ensure that the admin_peers endpoint deserializes correctly
  - the id obtained from the admin_peers RPC call matches the id from the admin_nodeInfo response for that peer
- more tests should be added for admin endpoints, maybe good for a future PR or issue
- another thing out of scope for this PR: genesis.json deserialization tests
simple (de)serialization tests for NodeInfo and PeerInfo
consider other admin_ endpoints to add - admin_peerEvents maybe?
- admin_peerEvents is of scope for this PR, will create an issue

PR Checklist

Added Tests
Added Documentation
Updated the changelog

Rjected · 2022-11-26T04:52:39Z

Currently fails the deserialize_node_info_post_merge test, ready for review after fixing that bug

Rjected · 2022-11-26T05:56:47Z

Tests pass and the bug is fixed, the bug was that from_int_or_hex wouldn't deserialize big numbers, luckily serde_json::Number can be used with the arbitrary-precision feature from serde_json

gakonst

nice work! nits

gakonst · 2022-11-27T20:50:51Z

ethers-providers/src/provider.rs

+        // wait for the geth dial loop to start and connect the static nodes
+        sleep(Duration::from_secs(20)).await;


this takes that long? can we instead poll the node like we poll ganache-cli instaed of hardcoding a wait time? it's gonna screw up our reth integration tests otherwise imo

yeah, added a method wait_to_add_peer which polls geth's stderr

another method would be polling the admin_peers endpoint, we might be able to accomplish this using admin_peerEvents in the future

gakonst · 2022-11-27T20:51:38Z

ethers-providers/src/admin.rs

+    use super::*;
+
+    #[test]
+    fn deserialize_peer_info() {


ethers-providers/Cargo.toml

gakonst · 2022-11-27T20:55:11Z

ethers-core/src/utils/mod.rs

+/// Deserializes the input into a U256, accepting both 0x-prefixed hex and decimal strings with
+/// arbitrary precision, defined by serde_json's [`Number`](serde_json::Number).
+pub fn from_int_or_hex<'de, D>(deserializer: D) -> Result<U256, D::Error>


should we have this in other desers too? no need to do now

some desers already use this, but it would be worth checking other types of rpc responses that might have decimal values

gakonst · 2022-11-27T20:55:45Z

ethers-core/src/utils/geth.rs

+    /// Disable discovery for the geth instance.
+    ///
+    /// This will put the geth instance into non-dev mode, discarding any previously set dev-mode
+    /// options.
+    #[must_use]
+    pub fn disable_discovery(mut self) -> Self {


nice. we should consider thinking of how can we use these options on geth to cause partitions intentionally and see how our failure recovery works

gakonst · 2022-11-28T20:04:10Z

@Rjected lets try to merge by eod?

Rjected · 2022-11-28T20:07:53Z

@gakonst sounds good, will prioritize

* add networking and peer related structs

* the type is better represented by a struct which can have either eth or snap

* should re-add when peer event endpoints are implemented

* the serde_json arbitrary_precision feature is necessary so the serde_json::Number variant of `IntOrHexOrBigNum` can be converted into a string and fed into U256::from_dec_string

* unnecessary since serde_json::Number handles smaller ints as well

Co-authored-by: Georgios Konstantopoulos <me@gakonst.com>

* the build can be updated by changing the GETH_BUILD env var

Rjected · 2022-11-29T04:35:27Z

Mysterious connection refused error in another test that also uses geth hmmm

* add method which dumps the unread stderr of the geth instance into a string

* bug is due to authrpc endpoint being in use

* did not work properly anyways

gakonst

amazing work

Rjected force-pushed the add-admin-nodeinfo branch from a970e35 to 44a04b8 Compare November 22, 2022 05:14

Rjected mentioned this pull request Nov 22, 2022

Add support for mock network testing paradigmxyz/reth#137

Closed

Rjected force-pushed the add-admin-nodeinfo branch from 99cd336 to c252fd5 Compare November 26, 2022 02:35

Rjected changed the title ~~feat(providers): add part of admin namespace~~ feat(providers): add a subset of admin namespace Nov 26, 2022

Rjected marked this pull request as ready for review November 26, 2022 02:54

Rjected force-pushed the add-admin-nodeinfo branch 4 times, most recently from 153a858 to 4fd5709 Compare November 26, 2022 06:37

gakonst suggested changes Nov 27, 2022

View reviewed changes

Rjected added 16 commits November 28, 2022 17:09

tell Geth to expose the admin namespace

0c0a99c

wip: add admin namespace support

a350324

* add networking and peer related structs

add rest of chain config fields

0fce96c

add datadir to geth

4adffd4

fix data dir ref

0562f0b

add dev flag to geth

26c0d66

set dev only if block_time is not set

2bedcd2

put mutually exclusive options in an enum

6a237b3

make block_time use the devmode enum

d6b9687

add p2p port to geth

fce6c40

add basic impls for admin endpoints

52a7a35

move from_int_or_hex to ethers-core utils

e28d1a1

fix nodeinfo protocol field

4cfaffb

* the type is better represented by a struct which can have either eth or snap

add chain id and discovery toggle for Geth

1033b13

remove PeerEvent

5e3eb65

* should re-add when peer event endpoints are implemented

simplify serde options for admin responses

2bb8b32

Rjected and others added 15 commits November 28, 2022 17:09

improve documentation for genesis module

567cf38

remove not(wasm) attributes on genesis module

6183c87

remove debugging printlns

d27d0bc

remove io::Read from provider tests

a345fa3

add failing post merge test case

2c3375d

add full mainnet nodeinfo for testing

7efbd36

support deserializing json bignums to U256

51b4638

* the serde_json arbitrary_precision feature is necessary so the serde_json::Number variant of `IntOrHexOrBigNum` can be converted into a string and fed into U256::from_dec_string

fix from_int_or_hex_opt doc string

302cb9c

remove third variant from IntOrHex

54ad70c

* unnecessary since serde_json::Number handles smaller ints as well

add comments for ids

ae22226

fix enode id type in admin_peers provider test

cce36a6

fix admin typo in Cargo.toml

20557f0

Co-authored-by: Georgios Konstantopoulos <me@gakonst.com>

add method to wait on a gethinstance adding a peer

7697f69

fix dial loop and wait_to_add_peer doc comments

e92dbf6

update geth

50ccfce

* the build can be updated by changing the GETH_BUILD env var

Rjected force-pushed the add-admin-nodeinfo branch from 8e49a76 to 50ccfce Compare November 28, 2022 22:10

Rjected added 4 commits November 28, 2022 18:18

wait for geth to exit on drop

af47a31

remove unnecessary wait

f6188d3

fix mid-handshake PeerInfo deserialization

548b360

remove println

dbb53a9

Rjected force-pushed the add-admin-nodeinfo branch from a6e740a to dbb53a9 Compare November 29, 2022 03:33

Rjected added 6 commits November 29, 2022 15:48

make tests less flaky

96c897c

handle discovery with the rest of the non dev opts

7c445e3

dump geth stderr to debug failing ci test

a66548d

* add method which dumps the unread stderr of the geth instance into a string

remove call_raw debug println

b71c71c

* bug is due to authrpc endpoint being in use

use unused port when authrpc port is not specified

9d7e23e

remove dump_stderr from GethInstance

8ca864c

* did not work properly anyways

gakonst approved these changes Nov 30, 2022

View reviewed changes

gakonst merged commit 4b68562 into gakonst:master Nov 30, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(providers): add a subset of admin namespace #1880

feat(providers): add a subset of admin namespace #1880

Rjected commented Nov 22, 2022 •

edited

Loading

Rjected commented Nov 26, 2022

Rjected commented Nov 26, 2022

gakonst left a comment

gakonst Nov 27, 2022

Rjected Nov 28, 2022

Rjected Nov 28, 2022

gakonst Nov 27, 2022

Rjected Nov 28, 2022

gakonst Nov 27, 2022

Rjected Nov 28, 2022 •

edited

Loading

gakonst Nov 27, 2022

gakonst commented Nov 28, 2022

Rjected commented Nov 28, 2022

Rjected commented Nov 29, 2022 •

edited

Loading

gakonst left a comment

		// wait for the geth dial loop to start and connect the static nodes
		sleep(Duration::from_secs(20)).await;

feat(providers): add a subset of admin namespace #1880

feat(providers): add a subset of admin namespace #1880

Conversation

Rjected commented Nov 22, 2022 • edited Loading

Motivation

Solution

PR Checklist

Rjected commented Nov 26, 2022

Rjected commented Nov 26, 2022

gakonst left a comment

Choose a reason for hiding this comment

gakonst Nov 27, 2022

Choose a reason for hiding this comment

Rjected Nov 28, 2022

Choose a reason for hiding this comment

Rjected Nov 28, 2022

Choose a reason for hiding this comment

gakonst Nov 27, 2022

Choose a reason for hiding this comment

Rjected Nov 28, 2022

Choose a reason for hiding this comment

gakonst Nov 27, 2022

Choose a reason for hiding this comment

Rjected Nov 28, 2022 • edited Loading

Choose a reason for hiding this comment

gakonst Nov 27, 2022

Choose a reason for hiding this comment

gakonst commented Nov 28, 2022

Rjected commented Nov 28, 2022

Rjected commented Nov 29, 2022 • edited Loading

gakonst left a comment

Choose a reason for hiding this comment

Rjected commented Nov 22, 2022 •

edited

Loading

Rjected Nov 28, 2022 •

edited

Loading

Rjected commented Nov 29, 2022 •

edited

Loading