ENR #586

jannikluhn · 2019-05-06T08:03:17Z

Closes #583. Still WIP: A couple of additional tests are needed and we have to use compressed public keys (working on ethereum/eth-keys#55 now).

How was it fixed?

There's an ENR class that exposes the ENR key value pairs via a dict interface, along the sequence number and signature. If the signature is None it is considered unsigned. A signed copy can be created with to_signed_enr.
There are special sedes classes ENRSedes and ENRContentSedes to serialize/deserialize an ENR with or without the signature. Values are (de)serialized as binaries, unless there's something better in ENR_KEY_SEDES_MAPPING.
Identity schemes (responsible for signing and validating signatures) are represented by children of ENRIdentityScheme which define a method to check if they can be used for a given ENR or not (usually by checking if the ENR contains a public key in the right format). The ENR will use the first matching identity scheme it can find (there will probably only be a few).

Cute Animal Picture

jannikluhn · 2019-05-20T10:13:37Z

Updated to use the new eth-keys API introduced in ethereum/eth-keys#56 and ethereum/eth-keys#58. I still have to add the new version in setup.py, but I can only do this once the new changes have been released. After that, the tests should pass.

I also restructured the code a little in order to have two separate classes for signed and unsigned ENRs (instead of an is_signed property and a signature that may be None).

hwwhww

Solid work! 👍
Need to fix linter.

p2p/discv5/enr.py

p2p/discv5/identity_schemes.py

p2p/discv5/enr.py

p2p/discv5/identity_schemes.py

pipermerriam

Can you open (or link me to an existing) larger meta-tracking issue to tie these various discv5 PR's together into a larger epoch so that it's easier to understand the big picture as I review these smaller components?

p2p/discv5/enr.py

p2p/discv5/identity_schemes.py

tests/p2p/discv5/test_enr.py

jannikluhn · 2019-05-26T13:31:46Z

Opened a meta issue here (I guess I should have started with that): #668

jannikluhn · 2019-05-26T14:55:37Z

I've addressed the feedback and now I'm waiting for ethereum/eth-keys#58 and a subsequent eth-keys release. Then I'll have a look at the outstanding CI errors.

pipermerriam · 2019-05-26T18:17:05Z

That suggests the verify_signature api should be changed if possible to either convert the exception case to return false. I'm on mobile so haven't really done my homework on feasibility or why the api was designed that way. Sent from ProtonMail mobile

…

-------- Original Message --------

On May 26, 2019, 4:07 PM, jannikluhn wrote: @jannikluhn commented on this pull request. --------------------------------------------------------------- In [p2p/discv5/identity_schemes.py](#586 (comment)): > + signature = private_key.sign_msg_non_recoverable(message) + return bytes(signature) + + @classmethod + def validate_signature(cls, enr: "ENR") -> None: + public_key = PublicKey.from_compressed_bytes(enr[cls.public_key_enr_key]) + signature = NonRecoverableSignature(enr.signature) + message = enr.get_signing_message() + + try: + is_valid = signature.verify_msg(message, public_key) + except BadSignature: + is_valid = False + + if not is_valid: + raise ValidationError("Invalid signature") verify_signature can either return False or raise a BadSignature, depending on the way the signature is invalid. So there isn't away to avoid the variable without copying the raise ValidationError line which I'd like to avoid. — You are receiving this because you commented. Reply to this email directly, [view it on GitHub](#586?email_source=notifications&email_token=AAGJHAVZOCEIGL3VUZMLFCDPXKKR3A5CNFSM4HK56BGKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOBZWUFBA#discussion_r287599483), or [mute the thread](https://github.com/notifications/unsubscribe-auth/AAGJHARB5O6C3IK65PIMSULPXKKR3ANCNFSM4HK56BGA).

fjl · 2019-06-06T14:30:41Z

Very nice to see Trinity will have ENR support :).

Note that EIP-778 was changed one more time to add "tcp6", "udp6" keys. They allow relaying IPv6-specific port numbers.

We have also decided to use base64url encoding (without padding) instead of standard base64.
Base64 without padding is not supported in the Python standard library, but you can use these
functions to do it: fjl/p2p-drafts@23ed342#diff-09ba11ec95dfcee8269304dd2cc0abf3R156

jannikluhn · 2019-06-06T15:45:28Z

Updated to latest version of ENR spec (thanks @fjl)
reworked tests: clear separation between ENR and identity scheme
cleaned up commit history

Unfortunately, now everything breaks due to a version conflict. Pretty much all eth-* libraries require eth_keys < 3.0.0, but here we need eth_keys >= 3.0.0. I guess there's no other way but going through all of them and make a new release? 3.0.0 should be backwards compatible.

carver · 2019-06-06T18:17:53Z

Unfortunately, now everything breaks due to a version conflict. Pretty much all eth-* libraries require eth_keys < 3.0.0, but here we need eth_keys >= 3.0.0. I guess there's no other way but going through all of them and make a new release? 3.0.0 should be backwards compatible.

Yeah, sorry, this is fixed in #699 and was released so it doesn't affect fresh installs. But it hasn't been merged in yet. I'll ping you when it gets merged in, and the rebase should be painless.

carver · 2019-06-06T18:59:26Z

@jannikluhn Okay, a rebase should fix the eth-keys issue now.

jannikluhn · 2019-06-07T07:28:26Z

@carver that didn't do the trick. The problem is that this PR needs eth-keys >= 3.0.0, everything else eth-keys < 3.0.0. So I think there's no other way than updating all of them to use < 4.0.0.

carver · 2019-06-07T18:15:54Z

Ah, right. @pipermerriam instead of propagating a chain of eth-keys releases up to trinity (I think at least these would need new releases: eth-account, web3, py-evm), maybe we can just do a patch release that includes everything that's in 0.3.0?

pipermerriam · 2019-06-07T18:34:47Z

v0.2.3 is now available and equal to v0.3.0

setup.py

carver · 2019-06-10T16:42:58Z

Hm, interesting, I think this is a legit issue. The new eth-keys is treating a valid signature as invalid?

_ test_rpc_against_fixtures[/home/circleci/repo/fixtures/BlockchainTests/bcValidBlockTest/SimpleTx3.json:SimpleTx3_Frontier:Frontier] _
[gw1] linux -- Python 3.6.8 /home/circleci/repo/.tox/py36-rpc-blockchain/bin/python

event_bus = <trinity.endpoint.TrinityEventBusEndpoint object at 0x7f2f8c0a39b0>
chain_fixture = {'_info': {'comment': '', 'filledwith': 'testeth 1.5.0.dev2-68+commit.4a43125d.dirty', 'lllcversion': 'Version: 0.5.0-...80832fefd8808454c98c8142a056e81f171bcc55a6ff8345e692c0f86e5b48e01b996cadc001622fb5e363b421880102030405060708c0c0', ...}
fixture_data = ('/home/circleci/repo/fixtures/BlockchainTests/bcValidBlockTest/SimpleTx3.json', 'SimpleTx3_Frontier', 'Frontier')

    @pytest.mark.asyncio
    async def test_rpc_against_fixtures(event_bus, chain_fixture, fixture_data):
        rpc = RPCServer(
            initialize_eth1_modules(MainnetFullChain(None), event_bus)
        )
    
        setup_result, setup_error = await call_rpc(rpc, 'evm_resetToGenesisFixture', [chain_fixture])
        # We need to advance the event loop for modules to be able to pickup the new chain
        await asyncio.sleep(0)
        assert setup_error is None and setup_result is True, "cannot load chain for {0}".format(fixture_data)  # noqa: E501
    
        await validate_accounts(rpc, chain_fixture['pre'])
    
        for block_fixture in chain_fixture['blocks']:
            should_be_good_block = 'blockHeader' in block_fixture
    
            if 'rlp_error' in block_fixture:
                assert not should_be_good_block
                continue
    
            block_result, block_error = await call_rpc(rpc, 'evm_applyBlockFixture', [block_fixture])
    
            if should_be_good_block:
>               assert block_error is None
E               AssertionError: assert 'Invalid Signature' is None

block_error = 'Invalid Signature'
block_fixture = {'blockHeader': {'bloom': '0x00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000...ce': '0x00', ...}, {'data': '0x', 'gasLimit': '0x5208', 'gasPrice': '0x01', 'nonce': '0x03', ...}], 'uncleHeaders': []}
block_result = None
chain_fixture = {'_info': {'comment': '', 'filledwith': 'testeth 1.5.0.dev2-68+commit.4a43125d.dirty', 'lllcversion': 'Version: 0.5.0-...80832fefd8808454c98c8142a056e81f171bcc55a6ff8345e692c0f86e5b48e01b996cadc001622fb5e363b421880102030405060708c0c0', ...}
event_bus  = <trinity.endpoint.TrinityEventBusEndpoint object at 0x7f2f8c0a39b0>
fixture_data = ('/home/circleci/repo/fixtures/BlockchainTests/bcValidBlockTest/SimpleTx3.json', 'SimpleTx3_Frontier', 'Frontier')
rpc        = <trinity.rpc.main.RPCServer object at 0x7f2f68721ba8>
setup_error = None
setup_result = True
should_be_good_block = True

jannikluhn · 2019-06-10T16:46:20Z

yeah, that's my fault, this PR should fix it: ethereum/eth-keys#62

jannikluhn · 2019-06-11T20:19:31Z

Finally everything is green 🎉

I think I've addressed all comments so far, but since the last review I've restructured and extended the tests a bit. Anyone wants to have another look? Otherwise this is ready from my side.

pipermerriam · 2019-06-11T20:22:29Z

p2p/discv5/enr.py

+    def serialize(cls, enr: "BaseENR") -> Tuple[bytes, ...]:
+        serialized_sequence_number = big_endian_int.serialize(enr.sequence_number)
+
+        sorted_key_value_pairs = sorted(enr.items(), key=lambda key_value: key_value[0])


I believe historically we've measured operator.itemgetter(0) to be faster and slightly more preferred.

pipermerriam · 2019-06-11T20:27:43Z

p2p/discv5/enr.py

+                                 serialized_enr: Sequence[bytes]) -> None:
+        duplicates = set(
+            key for index, key in enumerate(keys)
+            if key in keys[index + 1:]


I think this has some unfortunate time complexity. An itertools.Counter does a pretty nice job of both detecting duplicates and allowing easy reporting of which ones are duplicate.

duplicates = {key for key, num in itertools.Counter(keys).items() if num > 1}

That one-liner does the trick pretty concisely.

neat! changed it

pipermerriam · 2019-06-11T20:29:01Z

p2p/discv5/enr.py

+            )
+
+
+class BaseENR(Mapping[bytes, Any], ABC):


Are the value types truly Any or can that be constrained more?

For now it's just int and bytes, but in theory it can be anything -- we might want to add special types for, e.g., libp2p multiaddrs or public key objects in the future. So to keep it easily extendable I'd prefer to keep it Any.

pipermerriam · 2019-06-11T20:30:59Z

p2p/discv5/enr.py

+
+    def get_identity_scheme(self) -> Type[IdentityScheme]:
+        try:
+            identity_scheme_id = self[b"id"]


Maybe instead of having to do this try/except checking at runtime it should happen during initialization and be memoized somewhere private since it's probably very natural to do that at whatever place the b'id' key is validated to be present.

pipermerriam · 2019-06-11T20:36:45Z

p2p/discv5/identity_schemes.py

+    )
+
+
+identity_scheme_registry: Dict[bytes, Type["IdentityScheme"]] = {}


We probably still use this pattern in a number of places but I've grown to think that we should be using a registry pattern closer to something like this.

class Registry(collections.UserDict): def register_thing(self, thing) -> None: ... # business logic here self[thing_key] = thing default_registry = Registry() register_thing = default_registry.register_thing

Note that collections.UserDict gives us an easy mechanism for simple registries like this.

https://docs.python.org/3/library/collections.html#collections.UserDict

Note that this requires ALL apis that make use of the registry to have some mechanism for passing in a custom registry. This pattern should result in easier isolation during testing as well as more flexibility when a library needs to override the base registry. I also think this approach has minimal extra overhead with respect to implementation and user facing API since the actualy underlying Registry API can be maintained as private.

Sounds reasonable. I've implemented something like this, but I'm not sure if I fully understood. Please have a look if that's how you imagined it.

pipermerriam · 2019-06-11T20:38:52Z

tests/p2p/discv5/test_enr.py

+            b"value1",
+            b"id",
+        ]),
+    )


This should be parametrized using pytest.mark.parametrize so it runs as 4 different tests.

pipermerriam

This looks great. 👍

pipermerriam · 2019-06-12T16:30:19Z

p2p/discv5/identity_schemes.py

+        pass
+
+
+@default_identity_scheme_registry.register


You might expose the register function as a top level module item that users can import to register a new identity scheme but that's only relevant if you want to expose this as a public API. For now I think it's fine to keep it private.

pipermerriam · 2019-06-12T16:31:44Z

tests/p2p/discv5/test_enr.py

+
+
+@pytest.fixture
+def test_identity_scheme_registry(mock_identity_scheme):


because pytest is greedy I've tended to avoid naming anything in a module that pytest scrapes with test_ (or classes that start with Test) because it sometimes tries to run them as tests. Naming schemes to get around this that seem appropriate are just to call this identity_scheme_registry or identity_scheme_registry_for_testing

jannikluhn mentioned this pull request May 22, 2019

Discv5 Messages #650

Merged

hwwhww approved these changes May 23, 2019

View reviewed changes

p2p/discv5/enr.py Outdated Show resolved Hide resolved

p2p/discv5/identity_schemes.py Outdated Show resolved Hide resolved

p2p/discv5/enr.py Outdated Show resolved Hide resolved

hwwhww reviewed May 23, 2019

View reviewed changes

p2p/discv5/identity_schemes.py Show resolved Hide resolved

pipermerriam approved these changes May 24, 2019

View reviewed changes

jannikluhn mentioned this pull request May 26, 2019

Discv5 Wire Protocol Tracking Issue #668

Closed

18 tasks

jannikluhn force-pushed the enr branch 2 times, most recently from f64e91a to 2dd80e8 Compare June 6, 2019 15:36

carver mentioned this pull request Jun 6, 2019

Patch release of alpha.25, to merge in so that it's on master #699

Merged

jannikluhn force-pushed the enr branch from 2dd80e8 to 3c939e9 Compare June 7, 2019 07:04

jannikluhn mentioned this pull request Jun 7, 2019

Allow eth-keys versions >=0.3, <0.4 ethereum/eth-keyfile#23

Closed

carver reviewed Jun 7, 2019

View reviewed changes

setup.py Outdated Show resolved Hide resolved

jannikluhn force-pushed the enr branch from 2875f16 to da77672 Compare June 10, 2019 08:20

jannikluhn force-pushed the enr branch from ccb9acc to a334087 Compare June 11, 2019 19:47

pipermerriam approved these changes Jun 11, 2019

View reviewed changes

pipermerriam approved these changes Jun 12, 2019

View reviewed changes

jannikluhn force-pushed the enr branch from e99103e to da01638 Compare June 13, 2019 09:23

jannikluhn added 6 commits June 13, 2019 11:26

Implement v4 identity scheme

bff8e17

Implement ENR format

6d82dea

Require eth-keys>=0.2.4 for non recoverable sigs

af2ed2a

Minor code beautifications

11a3f63

Support multiple identity registries

a5f11b8

Avoid test prefix for pytest fixture

da01638

jannikluhn merged commit fece800 into ethereum:master Jun 13, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENR #586

ENR #586

jannikluhn commented May 6, 2019

jannikluhn commented May 20, 2019 •

edited

Loading

hwwhww left a comment •

edited

Loading

pipermerriam left a comment

jannikluhn commented May 26, 2019

jannikluhn commented May 26, 2019

pipermerriam commented May 26, 2019 via email

fjl commented Jun 6, 2019 •

edited

Loading

jannikluhn commented Jun 6, 2019

carver commented Jun 6, 2019

carver commented Jun 6, 2019

jannikluhn commented Jun 7, 2019

carver commented Jun 7, 2019

pipermerriam commented Jun 7, 2019

carver commented Jun 10, 2019

jannikluhn commented Jun 10, 2019

jannikluhn commented Jun 11, 2019

pipermerriam Jun 11, 2019

pipermerriam Jun 11, 2019

jannikluhn Jun 12, 2019

pipermerriam Jun 11, 2019

jannikluhn Jun 12, 2019

pipermerriam Jun 11, 2019

pipermerriam Jun 11, 2019

jannikluhn Jun 12, 2019

pipermerriam Jun 11, 2019

pipermerriam left a comment •

edited

Loading

pipermerriam Jun 12, 2019

pipermerriam Jun 12, 2019

		)


		identity_scheme_registry: Dict[bytes, Type["IdentityScheme"]] = {}



		@pytest.fixture
		def test_identity_scheme_registry(mock_identity_scheme):

		pass


		@default_identity_scheme_registry.register

ENR #586

ENR #586

Conversation

jannikluhn commented May 6, 2019

How was it fixed?

Cute Animal Picture

jannikluhn commented May 20, 2019 • edited Loading

hwwhww left a comment • edited Loading

Choose a reason for hiding this comment

pipermerriam left a comment

Choose a reason for hiding this comment

jannikluhn commented May 26, 2019

jannikluhn commented May 26, 2019

pipermerriam commented May 26, 2019 via email

fjl commented Jun 6, 2019 • edited Loading

jannikluhn commented Jun 6, 2019

carver commented Jun 6, 2019

carver commented Jun 6, 2019

jannikluhn commented Jun 7, 2019

carver commented Jun 7, 2019

pipermerriam commented Jun 7, 2019

carver commented Jun 10, 2019

jannikluhn commented Jun 10, 2019

jannikluhn commented Jun 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pipermerriam left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jannikluhn commented May 20, 2019 •

edited

Loading

hwwhww left a comment •

edited

Loading

fjl commented Jun 6, 2019 •

edited

Loading

pipermerriam left a comment •

edited

Loading