client-side mru signatures -- step 1/5

[jpeletier]

As agreed during Tuesday's roundtable review, I am fragmenting PR #704 in incremental steps to make it easier to review. The steps are these:

• Step 1 : (PR client-side mru signatures -- step 1/5 #732) Minimum set of changes for client-side MRU signatures to work + system-time based MRUs. This was the status roughly 8 days ago.
• Step 2: (PR client-side mru signatures -- step 2/5 #733) Documentation, refactors and new, more concrete unit tests for the new structures
• Step 3: (PR client-side mru signatures -- step 3/5 #734) Refactor LookupParams so that the different combinations are documented. Moved LookupParams to its own file. Reorder code and create separate files for a few structures
• Step 4 (PR client-side mru signatures -- step 4/5 #748): Final refactors. Refactor serialization for extensibility. Refactor Timestamp to a "Timestamp" struct rather than a lone uint64. More comments throughout.
• Step 5: (PR Client-side MRU signing -- step 5/5 #704) Rewrite CLI and simplify client and internal object structure according to comments to decouple resource creation from updating.

Steps 1-4 are created as PRs to mru-tmp-2 branch (PR #668) so the diff is viewed as incremental from that point, rather than from swarm-network-rewrite, which would include @nolash's work too and make it difficult to view what changes this PR exactly brings in.

This code is not fully refactored and "clean" as the one in PR #704. The objective of this PR is to capture the essence of this change and gather comments. Some of the issues you will see may already have been fixed.

For code that has moved, I will comment below where it has gone.

Thank you in advance for your time and patience in reviewing this. I hope you enjoy this feature!

[jpeletier]

This moved to updaterequest.go. At the end of the refactor in PR #704, it will end up where signatures actually take place.

[jpeletier]

This moved to updaterequest.go. At the end of the refactor in PR #704, it will end up where signatures actually take place.

[jpeletier]

All these fields move to either resourceUpdate or resourceMetadata.

[jpeletier]

We don't need this any more

[jpeletier]

This moved to resourcemetadata.go since it serializes a metadata chunk

[jpeletier]

This also moved to resourcemetadata.go for the same reason above

[jpeletier]

bye bye ENS! :-)

[jpeletier]

Removed signer and headerGetter since we are not using ENS and also not signing inside Handler any more.

[nolash]

Why is storeTimeout gone?

[jpeletier]

Hmm, what do you mean? I still see it in Handler

[nolash]

Sorry, I must have read the diff wrong.

[jpeletier]

Moved the hashPool as a shared singleton instance for the entire package so we can usit throughout mru. Package initializer init deals with creating the pool.

[nolash]

Fine, but I have a plan to move the hashpool to a separate package altogether common for all things swarm.

[jpeletier]

That's great, I was wondering if that hashpool concept could be shared throughout the whole app. 👍

[jpeletier]

Validator now also checks metadata chunks.
To be fixed: should check len(data) > 2 before looking at the first two bytes.

[nolash]

So now the "metadata chunk" isn't content addressed anymore? That's why we need the first two bytes (again)?

[jpeletier]

The metadata chunk is still content addressed, however looking at the first two bytes at least saves calculating a hash if the incoming chunk doesn't look like a metadata chunk at first sight.

I am fine either way. We can do a hash and if it matches we mark as valid. What option would you choose?

[nolash]

ah yes the update chunks are uint16 first two of course.

[jpeletier]

parseUpdate now returns a SignedResourceUpdate

[jpeletier]

SignedResourceUpdate.Verify() deals with making sure the SignedResourceUpdate has a valid signature

[jpeletier]

Ascertain ownership of the resource without having to lookup or query ENS.

In the later PR Steps, this is refactored so parseUpdate returns an error if all these checks fail, further simplifying the Validate handler.

[jpeletier]

Removed as the UpdateRequest structure takes care of these cases.

[jpeletier]

Moved all this to a new method of the new resourceMetadata structure. This method hashes and serializes the metadata in one go.
In later steps, this is further refactored, and newMetaChunk becomes a direct method of resourceMetadata

[jpeletier]

this [8:] thing was driving me crazy, so now UnmarshalBinary takes care of it.

[nolash]

(chunk.SData+8) <3

[jpeletier]

This check here is key to make sure we are creating an update that is ahead of what we already know exists

[nolash]

If I remember correctly code resembling this was also used in the LookupPrevious method(s). Could we share it?

[jpeletier]

Code in LookupPrevious doesn't look like this. What do you mean?

[nolash]

yes right, LookupPrevious has it the other way around.

[nolash]

Please remove the binary.

I still need to go through the resource_test.go file

[nolash]

name should probably be newAccountManager

[jpeletier]

This needs to be refactored, maybe a different PR. The reason this is here and with this name is because it is a blatant copy of node.makeAccountManager, a private method of the node package and I didn't want to touch that package. Our problem is that the resource cli is the only command that uses accounts and private keys, and all the code to handle that is part of the Swarm node stuff.

A quick solution if you think others would agree is simply making that method public (?)

We would need a refactor whereupon account management is extracted out of the node domain and can be shared by the cli as well.

What do you think?

[nolash]

We would need a refactor whereupon account management is extracted out of the node domain and can be shared by the cli as well.

Can we theoretically just sign with the RPC call?

[jpeletier]

You need a private key. The idea of copying this was to reuse the account management code. So people using the swarm binary as a client can benefit of having their private keys in the same way as if they were running a node.

We can also disable this, but then users would need to point --bzzaccount to an unencrypted private key file, or we could have an additional parameter where the user can specify the path to their private key.

[nolash]

This sounds like a wider discussion. Let's have a phone about this when all of this is done.

[jpeletier]

Yes, I agree

[nolash]

Why should there be an ephemeral dir?

[jpeletier]

That'd be a review question for whoever wrote that method ;-) see above to see what we do with this part of the code.

[nolash]

A separate PR then. I'll try to remember %)

[nolash]

In my opinion it is getting confusing to have all flags listed under the main command when some of them only are relevant for subcommands, like here. For example openssl has separate usage texts for different subcommands; openssl asn1parse --help openssl s_client --help. Maybe not in scope for this PR, but the relative obscurity of the resource functionality in cli, plus the fact that it's two levels of subcommands, highlights the issue even more.

[jpeletier]

I agree. The reason it is like that is that (for now), as far as I was able to see, the current cli parser captures all the flags and I couldn't put the --raw thing after the create or update command, which is what I would have liked. Do you know who could help us determine the best way to have a syntax like this:

swarm resource create --raw <name> <freq> <data>

The current cli parser would find the --raw after the create command and wouldn't let me have it there (screwed with the rest of the args)

[jpeletier]

Never mind, I found out how to do it. It is now fixed directly in #704

[nolash]

"Create and update Mutable Resources"?

[jpeletier]

Changed. You'll see it directly in #704.

[nolash]

--raw flag should be set per update, not per resource, no?

[jpeletier]

Yes. What can be confusing is that, for the cli user, the create command actually creates a resource and sends off the first update in one go. I was hoping to simplify the interaction with this strategy.

Another approach would be to have a create command to create a resource which just returns the manifest hash and then the user has to invoke update to send the first update, otherwise the resource wouldn't have any content.

I am fine either way. The second approach is "cleaner": one command creates, other command updates, but also more complicated for a user. What do you think?

[nolash]

Another approach would be to have a create command to create a resource which just returns the manifest hash and then the user has to invoke update to send the first update, otherwise the resource wouldn't have any content

Yeah I remember pondering this too, and I too think it's cleaner. Let's go for that.

[jpeletier]

OK, I'll implement this directly in #704.

[jpeletier]

@nolash Finished modifying the CLI to decouple resource creation from updating according to these comments. The changes are in the last two commits of #704.
Creating a resource: swarm resource create <frequency> [--name <name>] [--data <0x Hexdata> [--rawmru]]
Updating a resource: swarm resource update <Manifest Address or ENS domain> <0x Hexdata> [--rawmru]
as you can see from the syntax, adding data during resource creation is now optional, but kept as a convenient way to create & initialize in one command.

[nolash]

cheeky but I like it

[jpeletier]

B-)

[nolash]

hash a "new resourcehash" to get a "resourcehash" suggests slightly misleading namings

[jpeletier]

This is refactored in #704 as follows:

• New structure to hold the lookup components, UpdateLookup which contains version, period, rootAddr.
• resourcehash -> UpdateLookup.GetUpdateAddr()
• NewResourceHash is gone since the structure serializer is reused.

[nolash]

Please remove obsolete comment

[jpeletier]

gone. Multihash is now encoded explicitly as a flag.

[nolash]

Maybe calculate these guys as global package vars

[jpeletier]

Refactored in #704. Every structure has a very clear calculation of its length.

[nolash]

Why not use the multihash package?

[jpeletier]

ok. Fixed in #704 and removed this function

[nolash]

@zelig did you approve this earlier?

[jpeletier]

Note that you are only merging to the temp branch, so shouldn't be a big deal. 😉