Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Split blockchain package to reduce transient dependency packages #1409

Closed
jrick opened this issue Aug 19, 2018 · 5 comments
Closed

Split blockchain package to reduce transient dependency packages #1409

jrick opened this issue Aug 19, 2018 · 5 comments

Comments

@jrick
Copy link
Member

jrick commented Aug 19, 2018

dcrwallet depends on the following APIs of the blockchain package:

$ egrep -roh 'blockchain\.[[:alnum:]]+' | sort | uniq
blockchain.BigToCompact
blockchain.BuildMsgTxMerkleTreeStore
blockchain.CalcStakeVoteSubsidy
blockchain.CalcWork
blockchain.CheckProofOfWork
blockchain.CompactToBig
blockchain.IsCoinBaseTx
blockchain.NewSubsidyCache
blockchain.SubsidyCache

However, none of these require database access, and simply importing the blockchain package additionally pulls in a multitude of other dependencies, including transitive dependencies which do not pass tests.

$ go list -json github.com/decred/dcrd/blockchain
...
	"Deps": [
		"bufio",
		"bytes",
		"compress/flate",
		"compress/zlib",
		"container/list",
		"context",
		"crypto",
		"crypto/aes",
		"crypto/cipher",
		"crypto/ecdsa",
		"crypto/elliptic",
		"crypto/hmac",
		"crypto/internal/randutil",
		"crypto/internal/subtle",
		"crypto/rand",
		"crypto/sha1",
		"crypto/sha256",
		"crypto/sha512",
		"crypto/subtle",
		"encoding",
		"encoding/asn1",
		"encoding/base64",
		"encoding/binary",
		"encoding/hex",
		"encoding/json",
		"errors",
		"fmt",
		"github.com/agl/ed25519",
		"github.com/agl/ed25519/edwards25519",
		"github.com/dchest/blake256",
		"github.com/decred/base58",
		"github.com/decred/dcrd/blockchain/internal/dbnamespace",
		"github.com/decred/dcrd/blockchain/internal/progresslog",
		"github.com/decred/dcrd/blockchain/stake",
		"github.com/decred/dcrd/blockchain/stake/internal/dbnamespace",
		"github.com/decred/dcrd/blockchain/stake/internal/ticketdb",
		"github.com/decred/dcrd/blockchain/stake/internal/tickettreap",
		"github.com/decred/dcrd/chaincfg",
		"github.com/decred/dcrd/chaincfg/chainec",
		"github.com/decred/dcrd/chaincfg/chainhash",
		"github.com/decred/dcrd/database",
		"github.com/decred/dcrd/dcrec",
		"github.com/decred/dcrd/dcrec/edwards",
		"github.com/decred/dcrd/dcrec/secp256k1",
		"github.com/decred/dcrd/dcrec/secp256k1/schnorr",
		"github.com/decred/dcrd/dcrjson",
		"github.com/decred/dcrd/dcrutil",
		"github.com/decred/dcrd/txscript",
		"github.com/decred/dcrd/wire",
		"github.com/decred/slog",
		"golang.org/x/crypto/ripemd160",
		"golang_org/x/net/dns/dnsmessage",
		"hash",
		"hash/adler32",
		"internal/bytealg",
		"internal/cpu",
		"internal/nettrace",
		"internal/poll",
		"internal/race",
		"internal/singleflight",
		"internal/syscall/unix",
		"internal/testlog",
		"io",
		"io/ioutil",
		"math",
		"math/big",
		"math/bits",
		"math/rand",
		"net",
		"os",
		"os/user",
		"path/filepath",
		"reflect",
		"runtime",
		"runtime/cgo",
		"runtime/internal/atomic",
		"runtime/internal/sys",
		"sort",
		"strconv",
		"strings",
		"sync",
		"sync/atomic",
		"syscall",
		"text/tabwriter",
		"time",
		"unicode",
		"unicode/utf16",
		"unicode/utf8",
		"unsafe"
	],
...
	"TestImports": [
		"bytes",
		"compress/bzip2",
		"encoding/gob",
		"encoding/hex",
		"errors",
		"fmt",
		"github.com/decred/dcrd/blockchain/chaingen",
		"github.com/decred/dcrd/blockchain/stake",
		"github.com/decred/dcrd/chaincfg",
		"github.com/decred/dcrd/chaincfg/chainhash",
		"github.com/decred/dcrd/database",
		"github.com/decred/dcrd/database/ffldb",
		"github.com/decred/dcrd/dcrutil",
		"github.com/decred/dcrd/txscript",
		"github.com/decred/dcrd/wire",
		"io/ioutil",
		"math",
		"math/big",
		"math/rand",
		"os",
		"path/filepath",
		"reflect",
		"runtime",
		"sort",
		"strconv",
		"testing",
		"time"
	],
...
	"XTestImports": [
		"bytes",
		"fmt",
		"github.com/decred/dcrd/blockchain",
		"github.com/decred/dcrd/blockchain/fullblocktests",
		"github.com/decred/dcrd/chaincfg",
		"github.com/decred/dcrd/chaincfg/chainhash",
		"github.com/decred/dcrd/database",
		"github.com/decred/dcrd/database/ffldb",
		"github.com/decred/dcrd/dcrutil",
		"github.com/decred/dcrd/txscript",
		"github.com/decred/dcrd/wire",
		"io/ioutil",
		"math/big",
		"os",
		"path/filepath",
		"testing"
	]

The most concerning of these imports is the test import on ffldb. This causes various leveldb packages to be built and tested as part of a dcrwallet go test all, even though dcrwallet does not use leveldb at all in non-test builds.

To fix this issue, the blockchain package can be split to move the non-DB APIs into a different package. It does not necessarily need to be in a different module, as go test all will only test the packages used as part of a complete build, and not all packages in every dependent module.

edit: I should be clear here, the failing tests are not due to leveldb itself (all tests for imported packages of the goleveldb module pass), but some transitive deps required by these goleveldb packages do not pass.

@jrick jrick changed the title Split blockchain package Split blockchain package to reduce transient dependency packages Aug 19, 2018
@davecgh
Copy link
Member

davecgh commented Aug 20, 2018

I'm not against the concept in theory, however, I'm not really sure what a good name for such a package would be as those really are all things that apply to the blockchain. Also, there would need to be some considerations made in terms of the behavior flags since we don't want circular dependencies.

From what I see, those functions basically fall into the following categories:

  • Difficulty calculation and verification
  • Subsidy calculation (speaking of which, I have it planned as part of Multi-peer Checklist #1145 to expose the calculation functions from the cache instance itself instead of taking the cache as a parameter explicitly, which I think would make the separation much more natural)
  • Merkle root calculation (this really needs to be cleaned up at some point to perform the calculation in log space and only return the root as opposed to all of the intermediate hashes too)
  • Transaction type identification

@jrick
Copy link
Member Author

jrick commented Aug 20, 2018

I'm at a loss for a good package name as well, which is why I did not provide one :)

I don't think I understand the issue with blockchain.BehaviorFlags. As I look at the godoc documentation, I only see the constants used by the BlockChain.ProcessBlock method. There are other flags as well, but these are defined in other packages (like txscript.ScriptFlags).

@jrick
Copy link
Member Author

jrick commented Aug 20, 2018

Another thing to keep in mind is that not all of these funcs and types (and there are a lot of them I didn't know about, based on my limited usage of blockchain in dcrwallet) need to all move into a single package. There could be separate packages for transaction classification, for difficulty calculation, etc. while keeping all of them under the same module.

@davecgh
Copy link
Member

davecgh commented Aug 20, 2018

blockchain.CheckProofOfWork calls an unexported version which accepts a behavior flag with BFNone.

@davecgh
Copy link
Member

davecgh commented Dec 28, 2019

This has been implemented via the blockchain/standalone module.

@davecgh davecgh closed this as completed Dec 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants