search

lukechampine / us

An alternative interface to Sia
MIT License
55 stars 4 forks source link

Proposal: Remove dependency on NebulousLabs/Sia #13

Open lukechampine opened 5 years ago

lukechampine commented 5 years ago

us does not strongly depend on the NebulousLabs/Sia packages; most of the references are to small types like modules.NetAddress, types.BlockHeight, etc. However, even a single reference is sufficient to drag in the entire repo as a dependency, which significantly bloats the size of binaries that import us packages. This also impacts the us bindings: the resulting ObjC framework, for example, is 53 MB, which is just crazy.

Unfortunately, there are a few places where the dependency is hard to remove. In particular, wallet.ChainScanner needs to implement modules.ConsensusSetSubscriber, which means it must depend on the modules.ConsensusChange type. Additionally, if we substitute our own types for NebulousLabs/Sia types, it becomes harder to write code that interoperates with both us and NebulousLabs/Sia.

I propose that we make two changes:

To solve the wallet.ChainScanner problem, we would redefine the method to depend on a locally-defined copy of modules.ConsensusChange, and then call interop.ToConsensusSetSubscriber on it to make it satisfy modules.ConsensusSetSubscriber. This is viable because we only need to actually subscribe to the consensus set in binaries like walrus and during certain tests, so you only "pay for" the NebulousLabs/Sia dependency when you actually need it. Accordingly, the bindings should shrink enormously.

lukechampine commented 5 years ago

Follow up: I wasn't 100% sure that NebulousLabs/Sia was responsible for the large binaries, so I fact-checked myself using the goweight tool. This tool indicates that indeed, a sizeable percentage of a binary like user comes from NebulousLabs packages.

However, when I tried another tool, it indicated that NebulousLabs packages are only responsible for about 1.6% of the compiled binary.

The only way to truly be sure is to strip out the dependency, recompile, and see how much smaller the binary is. Unfortunately, that requires a fair amount of work.

Still, even if binaries don't shrink as much as I'd hoped, there are other reasons why removing the dependency is desirable. In particular, it drags a ton of packages into our go.sum, which makes a proper audit far more difficult. See the attached graph; if not for NebulousLabs/Sia, we would depend on a total of 5 non-stdlib packages (pkg/errors, aead/chacha20, bbolt, x/crypto, x/sys): deps

Of those, pkg/errors could be replaced with the new Go 1.13 wrapped errors, and aead/chacha20 could be replaced with x/crypto once https://github.com/golang/crypto/pull/108 is merged. bbolt could also conceivably be moved to a separate package, although that's probably overkill. At any rate, it would be nice to have only three non-stdlib dependencies.

jkawamoto commented 4 years ago

This is a significant issue for us, too. The problem is NebulousLabs/Sia tends to print a stack trace instead of panicking when some critical errors occur. For example, we got this:

goroutine 7358 [running]:
runtime/debug.Stack(0x44ff77, 0x0, 0xc000b5b268)
        /usr/local/Cellar/go/1.14.2_1/libexec/src/runtime/debug/stack.go:24 +0x9d
runtime/debug.PrintStack()
        /usr/local/Cellar/go/1.14.2_1/libexec/src/runtime/debug/stack.go:16 +0x22
gitlab.com/NebulousLabs/Sia/build.Critical(0xc000b5b310, 0x1, 0x1)
        /Users/junpei/pkg/mod/gitlab.com/!nebulous!labs/!sia@v1.4.7/build/critical.go:16 +0xaa
gitlab.com/NebulousLabs/Sia/types.Currency.Sub(0xc000b5bc00, 0xc000bc3680, 0x2, 0x7, 0x100, 0xc000bc3700, 0x2, 0x7, 0x0, 0x0, ...)
        /Users/junpei/pkg/mod/gitlab.com/!nebulous!labs/!sia@v1.4.7/types/currency.go:190 +0x14e
lukechampine.com/us/renter/proto.(*Session).RenewAndClearContract(0xc0000ba580, 0x1191000, 0xc0004c8258, 0x117f940, 0xc0004c8258, 0xc0004f4400, 0xc000aae600, 0x2, 0x7, 0x3fbe5, ...)
        /Users/junpei/pkg/mod/lukechampine.com/us@v0.16.1-0.20200429154232-11de3bc4411c/renter/proto/renew.go:289 +0xa82
lukechampine.com/us/renter/proto.RenewContract(0x1191000, 0xc0004c8258, 0x117f940, 0xc0004c8258, 0xa6d93141424e68de, 0x9d33a41fb271994, 0xe66fb7c5713620c8, 0x7b2dc55531564d00, 0xc0004f44b0, 0x40, ...)
        /Users/junpei/pkg/mod/lukechampine.com/us@v0.16.1-0.20200429154232-11de3bc4411c/renter/proto/renew.go:31 +0x271
...
Critical error: negative currency not allowed
Please submit a bug report here: https://gitlab.com/NebulousLabs/Sia/issues

This behaviour doesn't work for us because of these reasons:

Although we can customize this build package's behaviour with build tags, it requires to set debug,testing to panic instead of printing stack traces. However, those tags will break other packages for production.

lukechampine commented 4 years ago

In fairness, anything that triggers a build.Critical is a developer error that I should fix immediately. But yeah, I would use a normal panic for that.