HTTP API for mutable resources #204

nolash · 2018-01-19T19:48:50Z

This pr adds ResourceHandler API in the swarm HTTP layer, with a placeholder implementation of manifests. It introduces two new schemes: bzz-db and bzz-db-raw, which are for manifest view and raw data view of resource updates respectively.

GET methods map as follows:

* <scheme>:/<id>/ -  ResourceHandler.LookupLatest(<id>, true)
* <scheme>:/<id>/n - ResourceHandler.LookupHistorical(<id>, n, true)
* <scheme>:/<id>/n/m - ResourceHandler.LookupVersion(<id>, n, m, true)

POST path is always <scheme>:/<id>/ and the message body (current only raw data) is the data of the update.

<id> can be either ENS name or hash.

A new method ethclient.Client.BlockNumber() is added for convenience to enable passing the ensClient from swarm/swarm.go.NewSwarm(..) (otherwise we use an extra ?rpc.Client so we can access the eth_blockNumber RPC directly).

lmars · 2018-01-19T23:18:10Z

cmd/swarm/main.go

This is becoming unwieldy, NewSwarm should just read all those bzzconfig.XXX variables directly rather than passing them as arguments.

lmars · 2018-01-19T23:38:49Z

ethclient/ethclient.go

Shouldn't this be a big.Int like the backend function? Same for the return value.

Well, I don't like the numbers when you add another, no, wait, two other conversions with big.Int. So maybe I should keep the rpcclient then:

$ go test -v -cpu 4 . -bench . goos: linux goarch: amd64 BenchmarkLoInt-4 50000000 20.4 ns/op BenchmarkHiInt-4 30000000 38.2 ns/op BenchmarkLoBig-4 10000000 219 ns/op BenchmarkHiBig-4 5000000 299 ns/op BenchmarkLoBigViaInt64-4 10000000 188 ns/op BenchmarkHiBigViaInt64-4 10000000 268 ns/op

https://gist.github.com/nolash/67e5fc146b911712af44528819dd49c8

I don't like the numbers when you add another, no, wait, two other conversions with big.Int

I'm not sure what you mean, I'm saying pass big.Int into the rpc client so that it decodes the response into a big.Int (which is that the backend function actually returns) rather than here manually decoding into a uint64?

So maybe I should keep the rpcclient then

Why do you need to keep the rpcclient? This seems like a useful addition to ethclient no?

As for the benchmark, I'm not really sure what you are benchmarking here, are you concerned that decoding into a big.Int is slower than into a uint64? Why does it matter, surely we are not calling BlockNumber that often, it only changes every ~10s?

I'm more concerned with the assumption that BlockNumber is a uint64 when it isn't (it's a big.Int).

GetBlock() is called for every Resource creation, update and lookup for latest update. In the end I'm not how the use cases will be, and how often these calls will be performed.

I see your point that the middleware should reflect the data type of the backend, but I find it a bit bureaucratic to waste 10x extra cycles when it's not necessary just keep the form: I mean, do you think we will cross the max uint64 blocknumber anytime soon?

Actually, maybe the backend itself should not be big.Int for the blocknumber...

I see that rpc.LatestBlockNumber which the backend function encapulates is type rpc.BlockNumber which is even just a int64 ...

I'll look to see if we can make this hook better. It seems to me to be a real waste of resources.

To not get too out of context for this PR I'll implement as big.Int for nowbut will investigate how to slim it later.

GetBlock() is called for every Resource creation, update and lookup for latest update.

So then we should cache the return value and invalidate it when a new block is mined (which happens every ~10s), there is no need to continue to call an RPC method that is most likely to return the same value.

Actually, maybe the backend itself should not be big.Int for the blocknumber...

I guess this is the point I was trying to make, if an RPC method returns big.Int, then the client should also return the same thing, it's supposed to look transparent. If we think big.Int is wrong, it should be changed consistently everywhere.

@nolash doesn't look like this will be accepted anyway: ethereum/go-ethereum#15907

lmars · 2018-01-19T23:43:05Z

swarm/api/api.go

Why does this method have a key argument?

Also, why is it prefixed Db? Would LookupResource not be more appropriate?

My thinking was that bzz-resource-* scheme was a bit on the verbose side so I chose db instead, and felt the name in the API layer should reflect that name.

I have no strong feeling about this one way or the other.

I think db should be reserved for when Swarm actually has database functionality (e.g. indexing and querying).

lmars · 2018-01-19T23:46:46Z

swarm/api/config_test.go

Why is this commented out?

It's an apparently but unconfirmed obsolete parameter in HiveParams. I think the idea was that someone go through the changes of all the parameters that were weeded out to see if some of them were actually needed. But I guess maybe enough time has passed now that we can just drop it.

lmars · 2018-01-19T23:49:49Z

swarm/api/http/server.go

This should call s.BadRequest, explaining why the request was bad (e.g. "error parsing frequency from the request path").

lmars · 2018-01-20T00:33:15Z

swarm/storage/resource_test.go

It feels a little awkward to have to call teardownTest rather than the typical t.Fatal, typically tests just teardown in a defer, something like:

teardownTest := setupTest(t, ...) defer teardownTest() ... if err != nil { t.Fatal(err) }

lmars · 2018-01-20T00:37:00Z

swarm/testutil/http.go

Is it really necessary to start an RPC server for this? Should the resource handler not just accept an interface containing the methods it needs?

Very good point, sir.

lmars · 2018-01-20T00:38:13Z

swarm/testutil/http.go

Cleanup needs to happen before calling t.Fatal, otherwise the temporary directories will leak.

lmars · 2018-01-20T00:39:36Z

swarm/testutil/http.go

What is privatekey for?

lmars · 2018-01-20T00:41:03Z

swarm/testutil/http.go

Is this cleanup function being called?

nolash · 2018-01-20T18:17:28Z

@lmars thanks for the thorough review. A bit premature; it still has the "in progress" tag (even needs rebase) ;) but I'll take a pass and amend your comments.

lmars · 2018-01-21T13:22:54Z

ethclient/ethclient.go

I still don't understand why we are using a string? Can the response not be decoded directly into big.Int?

zelig · 2018-01-21T19:34:13Z

swarm/api/api.go

let's not call it Db anything please

fixed in 221b084

zelig · 2018-01-21T19:37:55Z

swarm/api/uri.go

let's find a different name for the scheme and functions reflecting it

zelig · 2018-01-21T19:38:55Z

swarm/storage/resource.go

so this layout will change according to what we discussed right?

yeah added in 3a90aee

zelig · 2018-01-21T19:42:58Z

swarm/storage/resource_test.go

much nicer this way...

zelig · 2018-01-21T19:45:20Z

very nice PR @nolash , cannot wait to have this working

lmars · 2018-01-22T21:15:50Z

ethclient/ethclient.go

It seems the go-ethereum team are against adding this, should we take a different approach as suggested?

Hmm, annoying. Thanks for catching this.

Are we going with a different approach?

lmars · 2018-01-22T21:16:45Z

swarm/api/api.go

return self.resource.GetContent(name)

lmars · 2018-01-22T21:19:40Z

swarm/api/http/server.go

I think ResourceUpdate should return a specific error to trigger a 401 response, otherwise this will just hide other errors.

lmars · 2018-01-22T21:22:23Z

swarm/api/http/server.go

If len(params) == 1 then isn't this going to panic?

lmars · 2018-01-22T21:22:56Z

swarm/api/http/server.go

No need for fmt.Sprintf here (nothing is being formatted).

lmars · 2018-01-22T21:35:08Z

swarm/storage/resource.go

We should comment why we are using context.Context in a non-typical way (and warn users to change their expectations).

lmars · 2018-01-22T21:37:37Z

swarm/storage/resource.go

Rather than using a cursor, you could just decode the chunk directly into a struct using binary.Read

I really have to get myself a hobby letting my write some C ;)

Actually, the data is not fixed size, and the https://golang.org/pkg/encoding/binary/#Read state:

func Read(r io.Reader, order ByteOrder, data interface{}) error Read reads structured binary data from r into data. Data must be a pointer to a fixed-size value or a slice of fixed-size values.

So I guess I get to keep my nerdy cursor? :)

Nice to know about this, though!

lmars · 2018-01-22T21:40:12Z

swarm/storage/resource.go

return idna.ToASCII(name)

lmars · 2018-01-22T21:41:20Z

swarm/storage/resource.go

Consider using sync.Pool to make this more concurrent.

You mean have several hashers in the pool? Never used that construct.

lmars · 2018-01-22T21:44:53Z

swarm/testutil/http.go

Is this being used?

yes, it's provides the fake block number retriever, same as in swarm/storage/resource_test.go

It doesn't seem to be instantiated anywhere.

It is in line 76, when creating the ResourceHandler

That's fakeBackend not FakeRPC

right you are, thanks

gbalint · 2018-01-23T12:01:27Z

swarm/api/api.go

Check err first, rsrc can be nil if there is an error, this can panic

gbalint · 2018-01-23T12:09:30Z

swarm/api/http/server_test.go

Let's add a loglevel command line flag, and then it is not disturbing at all, like here: https://github.com/ethersphere/go-ethereum/blob/swarm-network-rewrite-syncer/swarm/storage/common_test.go#L37

gbalint · 2018-01-23T12:10:25Z

swarm/api/http/server_test.go

No need for else after t.Fatal(err)

gbalint · 2018-01-23T12:10:32Z

swarm/api/http/server_test.go

No need for else after t.Fatal(err)

gbalint · 2018-01-23T12:15:19Z

swarm/api/http/server.go

@lmars but the case branch continues and tries s.api.ResourceLookup with the version which it failed to parse

Add ethclient.Client.BlockNumber method for access to current block number (needed by ResourceHandler) Add placeholder manifest support

ethclient: Omit string conversion detour

nolash · 2018-01-23T14:24:38Z

@lmars can we merge?

lmars · 2018-01-23T15:36:07Z

@nolash form a cursory glance, there are still a few comments which haven't been addressed, is there a rush to get this merged? If there is, feel free to merge but the comments will need to be addressed before merging upstream.

nolash · 2018-01-23T15:37:05Z

I thought I did them all. Which ones are you thinking of? The comments still open on this page are addressed, I believe.

lmars · 2018-01-23T15:37:50Z

I'll do another review...

nolash · 2018-01-23T15:46:14Z

Great @lmars

lmars · 2018-01-23T15:38:19Z

ethclient/ethclient.go

Are we going with a different approach?

lmars · 2018-01-23T15:41:47Z

swarm/api/http/server_test.go

Are we keeping this then?

lmars · 2018-01-23T15:43:42Z

swarm/api/http/server_test.go

+	keybytes := make([]byte, common.HashLength)
+	copy(keybytes, []byte{42})
+	srv.Hasher.Reset()
+	srv.Hasher.Write([]byte(fmt.Sprintf("%x", keybytes)))


Is it correct to be writing the hex representation of the key to the hasher, rather than the bytes themselves?

For example, I'd expect this to be:

srv.Hasher.Write(keybytes)

It's correct because we're getting it in hex from the HTTP server.

I'm not sure I follow. So you have keybytes which is the "key" of the resource. Why would I want to create a cryptographic hash of the hex representation of keybytes rather than just the hash of keybytes? It feels like a waste of time (the hex representation is twice as many bytes)?

Ah, I think I understand the confusion. These first "keybytes" are merely the "name" of the resource. The resource name is always hashed, and the hash is the key of the root chunk. In the backend, as you see, there are no double hashes. But in the test we need to to make the data match.

If you think it should be clearer, I guess we could just use a predictable string instead as "name".

lmars · 2018-01-23T15:44:44Z

swarm/api/http/server_test.go

+	if !bytes.Equal(b, []byte(keybyteshash)) {
+		t.Fatalf("resource update hash mismatch, expected '%s' got '%s'", keybyteshash, b)
+	}
+	resp.Body.Close()


This should be deferred right after getting the response so that it is called even if there is an error.

lmars · 2018-01-23T15:46:55Z

swarm/api/http/server_test.go

+	log.Root().SetHandler(log.CallerFileHandler(log.LvlFilterHandler(log.LvlTrace, log.StreamHandler(os.Stderr, log.TerminalFormat(true)))))
+}
+
+// \TODO if create -> get -> update -> get, the last get with return 1.1 because 1.2 retrieve is still pending


Has this been addressed?

lmars · 2018-01-23T15:47:35Z

swarm/api/http/server_test.go

+	if !bytes.Equal(data, b) {
+		t.Fatalf("Expected body '%x', got '%x'", data, b)
+	}
+	resp.Body.Close()


There should be test coverage of the "version" part of the URL.

lmars · 2018-01-23T15:52:00Z

swarm/storage/resource.go

-	err := self.rpcClient.Call(&currentblock, "eth_blockNumber")
+func (self *ResourceHandler) GetBlock() (uint64, error) {
+	ctx, cancel := context.WithCancel(self.ctx)
+	defer cancel()


It doesn't really make sense to create a context with a deferred cancel which will only be called when the API method being called with the context returns.

Clear redundant addition of BlockNumber in ethclient

nolash · 2018-01-23T20:50:44Z

Removing the cancel call results in go vet complaining:

swarm/storage/resource.go:628: the cancel function returned by context.WithCancel should be called, not discarded, to avoid a context leak

(#204 (comment))

lmars · 2018-01-23T22:35:41Z

@nolash

Removing the cancel call results in go vet complaining:

Did you remove the context creation? The idea of a context is that it is passed in to API boundaries.

In this case it would seem like the ResourceXXX methods should take a context argument, and that should be passed through to the rpc client. This gives the calling user the ability to cancel everything if it so wishes (e.g. http.Request.Context for HTTP requests, which is cancelled if the client drops the TCP connection).

nolash · 2018-01-24T01:48:34Z

@lmars done

lmars · 2018-01-25T12:17:21Z

swarm/swarm.go


-	// set up high level api
+	// SET UP high level api
+	fmt.Fprintf(os.Stderr, "privkey %v\nswap %v\nswapendabled %v\n", self.privateKey, config.Swap, config.SwapEnabled)


Let's not print the private key 😉

lol let's not

lmars

LGTM (please remove the private key debug log though)

nolash self-assigned this Jan 19, 2018

nolash added in progress good first issue and removed good first issue labels Jan 19, 2018

lmars reviewed Jan 20, 2018

View reviewed changes

lmars reviewed Jan 21, 2018

View reviewed changes

ethclient/ethclient.go Outdated

Copy link

lmars Jan 21, 2018

Choose a reason for hiding this comment

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

I still don't understand why we are using a string? Can the response not be decoded directly into big.Int?

zelig reviewed Jan 21, 2018

View reviewed changes

nolash force-pushed the swarm-mutableresources-apinew branch from 3a90aee to 04243fc Compare January 22, 2018 18:11

nolash added ready for another review and removed in progress labels Jan 22, 2018

lmars reviewed Jan 22, 2018

View reviewed changes

gbalint suggested changes Jan 23, 2018

View reviewed changes

nolash force-pushed the swarm-mutableresources-apinew branch from 930ad73 to cf14993 Compare January 23, 2018 14:22

nolash added 15 commits January 23, 2018 15:23

swarm/storage: Simplify code, correct content hashing

3d92c93

swarm/storage: Remove signatures from non-validated resources

cf191d3

swarm: Add base api for mutable resources

13543f4

swarm/api: Add all lookup types + public getblock

81ec1f9

swarm/api: Add raw form to api+server WIP

1bf4f4a

swarm, cmd/swarm, ethclient: Fullstack mut.rsrc. w api

249e1a8

Add ethclient.Client.BlockNumber method for access to current block number (needed by ResourceHandler) Add placeholder manifest support

swarm/, cmd/swarm: Amend comments from @lmars PR 204

a16d0af

swarm/api: Add contenttype contingent resolve manifest -> rsrc

11c3b4c

swarm/api: Rename Db -> Resource

80f539c

swarm/storage, ethclient: Move signature to end of chunk data

aff40c4

ethclient: Omit string conversion detour

swarm: Cleanup after rebase on swarm-mutableresources-extsign

703051b

swarm/storage, swarm/api: Change noparam fmt.Errorf -> errors.New

c2e0be6

swarm/api/http: Test result data

400b7d6

swarm: Amend comments from @lmars PR 204 second review, part I

0455925

swarm/storage: Correct channel for waiting on chunk put

a32681c

nolash added 3 commits January 23, 2018 15:23

swarm/storage: Implement resourcehandler hashers as sync.Pool

94303aa

swarm/api: Remove faulty manifest handling + add rsrc create keycheck

edacd4b

swarm/api: Amend @gbalint comments PR 204 + args dep test loglvl

690522b

nolash force-pushed the swarm-mutableresources-apinew branch from cf14993 to 690522b Compare January 23, 2018 14:23

lmars reviewed Jan 23, 2018

View reviewed changes

swarm, ethclient: Add version test for http api

88ed2d4

Clear redundant addition of BlockNumber in ethclient

swarm/api, swarm/storage: Delinting

4a7b5b8

swarm/api, swarm/storage: Fullstack contexts

add8971

nolash added 3 commits January 24, 2018 19:12

swarm/fuse: Update swarm/api:Api.NewAPI constructor

8d7bf3f

swarm: Add missing privatekey from swarm config w/o swap

de74542

swarm/storage: Delint

f38859d

nolash force-pushed the swarm-mutableresources-apinew branch from d185f1f to f38859d Compare January 25, 2018 01:03

This was referenced Jan 25, 2018

Add granular error reporting in mutable resources #212

Merged

Infer multihash resource update content when datalength header is 0 #214

Merged

lmars reviewed Jan 25, 2018

View reviewed changes

lmars approved these changes Jan 25, 2018

View reviewed changes

gbalint added resource update labels Jan 25, 2018

swarm: Remove privkey log entry

ef5eeb6

nolash merged commit 85e2318 into swarm-network-rewrite Jan 25, 2018

gbalint deleted the swarm-mutableresources-apinew branch May 25, 2018 14:48

HTTP API for mutable resources #204

HTTP API for mutable resources #204

Uh oh!

Conversation

nolash commented Jan 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lmars Jan 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nolash Jan 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nolash Jan 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nolash Jan 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nolash commented Jan 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zelig commented Jan 21, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nolash commented Jan 19, 2018 •

edited

Loading

lmars Jan 19, 2018 •

edited

Loading

nolash Jan 20, 2018 •

edited

Loading

nolash Jan 20, 2018 •

edited

Loading

nolash Jan 20, 2018 •

edited

Loading

nolash commented Jan 20, 2018 •

edited

Loading

nolash Jan 22, 2018 •

edited

Loading