standalone binary for chain export and inspection #3594

frrist · 2019-10-25T23:34:05Z

Motivation

Create a standalone binary for exporting a blockchain from a go-filecoin repo to a car file.

anorth · 2019-10-27T01:11:52Z

This appears to duplicate a lot of code that already exists for traversing and exporting. The intention of "standalone" is not that it doesn't depend on existing code, just that it can run without requiring a daemon.

For #3555 the head/s must be read from the chain store. The CLI argument should be optional, defaulting to whatever the chain store has stored as the head (and later when 3555 is addressed, all of the heads).

frrist · 2019-10-28T19:13:23Z

Not sure what happened with CI here, going to ignore it until I moved this from draft to "ready".

# pkg-config --cflags  -- /go/src/github.com/filecoin-project/go-filecoin/go-bls-sigs/libbls_signatures.pc
Failed to open '/go/src/github.com/filecoin-project/go-filecoin/go-bls-sigs/libbls_signatures.pc': No such file or directory
No package '/go/src/github.com/filecoin-project/go-filecoin/go-bls-sigs/libbls_signatures.pc' found
pkg-config: exit status 1
# pkg-config --cflags  -- /go/src/github.com/filecoin-project/go-filecoin/go-sectorbuilder/sector_builder_ffi.pc /go/src/github.com/filecoin-project/go-filecoin/go-sectorbuilder/sector_builder_ffi.pc
Failed to open '/go/src/github.com/filecoin-project/go-filecoin/go-sectorbuilder/sector_builder_ffi.pc': No such file or directory
No package '/go/src/github.com/filecoin-project/go-filecoin/go-sectorbuilder/sector_builder_ffi.pc' found
Failed to open '/go/src/github.com/filecoin-project/go-filecoin/go-sectorbuilder/sector_builder_ffi.pc': No such file or directory
No package '/go/src/github.com/filecoin-project/go-filecoin/go-sectorbuilder/sector_builder_ffi.pc' found
pkg-config: exit status 1
FAIL	github.com/filecoin-project/go-filecoin/actor [build failed]
FAIL	github.com/filecoin-project/go-filecoin/actor/builtin/account [build failed]
FAIL	github.com/filecoin-project/go-filecoin/actor/builtin/miner [build failed]
FAIL	github.com/filecoin-project/go-filecoin/actor/builtin/storagemarket [build failed]
FAIL	github.com/filecoin-project/go-filecoin/block [build failed]

frrist · 2019-10-31T16:08:25Z

tools/chain-util/pkg/export/export.go

+func newChainStateCollector(dserv format.DAGService) *chainStateCollector {
+	return &chainStateCollector{
+		dagserv: dserv,
+	}
+}
+
+type chainStateCollector struct {
+	dagserv format.DAGService // Provides access to state tree.
+	state   []format.Node
+}
+
+// collectState recursively walks the state tree starting with `stateRoot` and returns it as a slice of IPLD nodes.
+// Calling this method does not have any side effects.
+func (csc *chainStateCollector) collectState(ctx context.Context, stateRoot cid.Cid) ([]format.Node, error) {
+	dagNd, err := csc.dagserv.Get(ctx, stateRoot)
+	if err != nil {
+		return nil, errors.Wrapf(err, "failed to load stateroot from dagservice %s", stateRoot)
+	}
+	csc.addState(dagNd)
+	seen := cid.NewSet()
+	for _, l := range dagNd.Links() {
+		if err := dag.Walk(ctx, csc.getLinks, l.Cid, seen.Visit); err != nil {
+			return nil, errors.Wrapf(err, "dag service failed walking stateroot %s", stateRoot)
+		}
+	}
+	return csc.state, nil
+
+}
+
+func (csc *chainStateCollector) getLinks(ctx context.Context, c cid.Cid) ([]*format.Link, error) {
+	nd, err := csc.dagserv.Get(ctx, c)
+	if err != nil {
+		return nil, errors.Wrapf(err, "failed to load link from dagservice %s", c)
+	}
+	csc.addState(nd)
+	return nd.Links(), nil
+
+}
+
+func (csc *chainStateCollector) addState(nd format.Node) {
+	csc.state = append(csc.state, nd)
+}


Copy-Pasta from here
https://github.com/filecoin-project/go-filecoin/blob/4f71dca6ec0a18e3b19445c39ef06033e51a859e/internal/app/go-filecoin/plumbing/cst/chain_state.go#L233-L277

I'm not happy about this code duplication, but it seems better than exporting newChainStateCollector from chain_state.go, thoughts?

Could you elaborate on why importing from chain_state.go is worse? Without your context importing seems like the better choice. You could put this type in the chain package or in a new ipld/dag or the util package if you don't want to export free functions from plumbing.

Mainly because I didn't like the idea of exporting a free function from plumbing. I will move this into dag plumbing exposed as a RecursiveGet method.

codecov-io · 2019-10-31T18:02:03Z

Codecov Report

Merging #3594 into master will decrease coverage by <1%.
The diff coverage is 8%.

@@          Coverage Diff           @@
##           master   #3594   +/-   ##
======================================
- Coverage      47%     47%   -1%     
======================================
  Files         270     270           
  Lines       17259   17281   +22     
======================================
  Hits         8151    8151           
- Misses       8001    8023   +22     
  Partials     1107    1107

frrist · 2019-11-01T21:38:57Z

go.mod

@@ -62,7 +60,6 @@ require (
 	github.com/libp2p/go-libp2p-circuit v0.1.3
 	github.com/libp2p/go-libp2p-core v0.2.4-0.20191022172111-a5bf2487c11d
 	github.com/libp2p/go-libp2p-kad-dht v0.1.1
-	github.com/libp2p/go-libp2p-peer v0.2.0
 	github.com/libp2p/go-libp2p-peerstore v0.1.3


red in this file is from running go mod tidy

ZenGround0

Looking good. I want this tool to do more to prevent accidental user datastore corruption.

ZenGround0 · 2019-11-08T16:01:33Z

tools/chain-util/pkg/export/export.go

+	log.Infof("opening filecoin datastore: %s", repoPath)
+	badgerPath := filepath.Join(repoPath, "badger/")
+	log.Infof("opening badger datastore: %s", badgerPath)
+	badgerDS, err := badgerds.NewDatastore(badgerPath, badgerOpt)


Since we are not using the go-filecoin/repo abstraction there doesn't seem to be any protection against multiple processes obtaining a handle to a datastore wrapping the same path.

I'm a bit worried that this could lead to accidental corruption if someone runs this tool while a daemon is running.

Lowest effort solution is to include an all CAPs user facing warning somewhere.

I think "the right thing" here is to open up a go-filecoin repo abstraction and then grab the datastores with Datastore and ChainDatastore. Even if we're lucky and concurrent read-read and read-write works for badger right now it adds a hidden requirement to our datastore choice that could erode with time.

Since we are not using the go-filecoin/repo abstraction there doesn't seem to be any protection against multiple processes obtaining a handle to a datastore wrapping the same path.

Since we are using badger to open the repo it will fail to open if a repo lock exists, from the linked README.md:

Please note that Badger obtains a lock on the directories so multiple processes cannot open the same database at the same time.

Here is what a user will see if they attempt to run this binary on a repo that currently has a lock held by another (go-filecoin) process.

./chain-util export --repo=/home/frrist/.filecoin/ --out=/tmp/test.car 2019-11-11T11:25:10.586-0800 INFO chain-util/export export/export.go:44 opening repo: /home/frrist/.filecoin/ 2019-11-11T11:25:10.586-0800 WARN chain-util chain-util/main.go:75 Cannot acquire directory lock on "/home/frrist/.filecoin/badger". Another process is using this Badger database.: resource temporarily unavailable

Also note that we are opening the repo in read-only mode:
https://github.com/filecoin-project/go-filecoin/pull/3594/files#diff-2bbea98c52d3e94072ef6b7ba1a9250dR27-R29

That's good! It moves this from a correctness to a bitrot thing. It still doesn't address this part of the comment:

Even if we're lucky and concurrent read-read and read-write works for badger right now it adds a hidden requirement to our datastore choice that could erode with time.

Can we assume that all ipfs datastore implementations MUST repo lock? If we can, great. If not the moment we pivot to another datastore or we open up user configured datastores we/users are going to need to satisfy a hidden requirement for interop with this tool which means there's some odds somebody won't satisfy this requirement and they'll be left wondering why the tool is corrupting their datastore.

Grabbing the filecoin repo lock enforces safety across all future datastore implementations.

I think I might have misunderstood the initial comment, let me try again.

Since we are not using the go-filecoin/repo abstraction

Meaning since we are not calling the go-filecoin method OpenFSRepo, correct?

doesn't seem to be any protection against multiple processes obtaining a handle to a datastore wrapping the same path

There is protection against multiple processes getting a handle to a datastore (as I have shown this is provided by the badger datastore package), however since go-filecoin creates is own repo lock and since we are not checking for it to exists here, we are left open to possible corruption in the event that we change our datastore/repo implementation. Do I understand this correctly?

Is the ask that a check for filecoin repo lock is added? (as is done here)

Yeah this is all exactly what I'm thinking. If we use openfsrepo we get the guarantee of go-filecoin repo locking preventing contention with other software that opens the repo (go-filecoin, the migration tool...) regardless of what datastore we use.

Originally I wasn't sure if badger had its own protection which is why I blocked approval and why I approved when you told me that it does.

ZenGround0 · 2019-11-08T16:06:20Z

tools/chain-util/pkg/export/export.go

+func newChainStateCollector(dserv format.DAGService) *chainStateCollector {
+	return &chainStateCollector{
+		dagserv: dserv,
+	}
+}
+
+type chainStateCollector struct {
+	dagserv format.DAGService // Provides access to state tree.
+	state   []format.Node
+}
+
+// collectState recursively walks the state tree starting with `stateRoot` and returns it as a slice of IPLD nodes.
+// Calling this method does not have any side effects.
+func (csc *chainStateCollector) collectState(ctx context.Context, stateRoot cid.Cid) ([]format.Node, error) {
+	dagNd, err := csc.dagserv.Get(ctx, stateRoot)
+	if err != nil {
+		return nil, errors.Wrapf(err, "failed to load stateroot from dagservice %s", stateRoot)
+	}
+	csc.addState(dagNd)
+	seen := cid.NewSet()
+	for _, l := range dagNd.Links() {
+		if err := dag.Walk(ctx, csc.getLinks, l.Cid, seen.Visit); err != nil {
+			return nil, errors.Wrapf(err, "dag service failed walking stateroot %s", stateRoot)
+		}
+	}
+	return csc.state, nil
+
+}
+
+func (csc *chainStateCollector) getLinks(ctx context.Context, c cid.Cid) ([]*format.Link, error) {
+	nd, err := csc.dagserv.Get(ctx, c)
+	if err != nil {
+		return nil, errors.Wrapf(err, "failed to load link from dagservice %s", c)
+	}
+	csc.addState(nd)
+	return nd.Links(), nil
+
+}
+
+func (csc *chainStateCollector) addState(nd format.Node) {
+	csc.state = append(csc.state, nd)
+}


Could you elaborate on why importing from chain_state.go is worse? Without your context importing seems like the better choice. You could put this type in the chain package or in a new ipld/dag or the util package if you don't want to export free functions from plumbing.

ZenGround0 · 2019-11-08T16:16:09Z

tools/chain-util/pkg/export/export.go

+	return chain.Export(ctx, ce.Head, ce, msgStore, ce, ce.out)
+}
+
+// GetTipSet gets all the TipSet for a given TipSetKey from the ChainExporter blockstore.


nit: gets the TipSet

ZenGround0

Leaving approved but strong preference for grabbing filecoin repo lock for reasons provided.

to be called by chain_state plumbing

frrist self-assigned this Oct 25, 2019

frrist mentioned this pull request Oct 25, 2019

init accepts a car file with full chain #3595

Merged

frrist force-pushed the feat/chain-export-bin branch from e7a7047 to fc46032 Compare October 26, 2019 00:09

frrist requested a review from anorth October 26, 2019 00:11

frrist force-pushed the feat/chain-export-bin branch from fc46032 to 5f00505 Compare October 26, 2019 00:37

frrist force-pushed the feat/chain-export-bin branch from 5f00505 to 6b51956 Compare October 30, 2019 22:07

frrist changed the title ~~[wip] standalone binary for chain export and inspection~~ standalone binary for chain export and inspection Oct 31, 2019

frrist marked this pull request as ready for review October 31, 2019 01:40

frrist commented Oct 31, 2019

View reviewed changes

frrist force-pushed the feat/chain-export-bin branch from 6b51956 to 57f83ea Compare November 1, 2019 20:59

frrist commented Nov 1, 2019

View reviewed changes

frrist requested a review from ZenGround0 November 5, 2019 18:10

ZenGround0 requested changes Nov 8, 2019

View reviewed changes

ZenGround0 self-requested a review November 11, 2019 21:51

ZenGround0 approved these changes Nov 11, 2019

View reviewed changes

frrist added 12 commits November 12, 2019 12:18

initial impl of chain export bin with statetree

f29c367

export HeadKey from chain store

077701f

removed duplicated export code

453c94f

fix linter

d4f96bb

go mod tidy

1212f17

go mod tidy

b588f87

add RecursiveGet method to dag plumbing

56a08c9

to be called by chain_state plumbing

call dag plumbing in chain-util export

b5cb850

fixup! add RecursiveGet method to dag plumbing

aa951cb

fixup! add RecursiveGet method to dag plumbing

167da99

fixup! call dag plumbing in chain-util export

117d5ef

go mod tidy

bdaec20

frrist force-pushed the feat/chain-export-bin branch from 20bd263 to bdaec20 Compare November 12, 2019 20:21

frrist added 3 commits November 13, 2019 17:17

Merge branch 'master' into feat/chain-export-bin

1a7aa73

Merge branch 'master' into feat/chain-export-bin

1ff4b09

Merge branch 'master' into feat/chain-export-bin

29e2184

frrist merged commit 47a315e into master Nov 14, 2019

frrist deleted the feat/chain-export-bin branch November 14, 2019 18:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

standalone binary for chain export and inspection #3594

standalone binary for chain export and inspection #3594

frrist commented Oct 25, 2019

anorth commented Oct 27, 2019

frrist commented Oct 28, 2019

frrist Oct 31, 2019

ZenGround0 Nov 8, 2019

frrist Nov 11, 2019

codecov-io commented Oct 31, 2019 •

edited

frrist Nov 1, 2019

ZenGround0 left a comment

ZenGround0 Nov 8, 2019

frrist Nov 11, 2019

ZenGround0 Nov 11, 2019

ZenGround0 Nov 11, 2019

frrist Nov 11, 2019 •

edited

ZenGround0 Nov 12, 2019

ZenGround0 Nov 8, 2019

ZenGround0 Nov 8, 2019

ZenGround0 left a comment

standalone binary for chain export and inspection #3594

standalone binary for chain export and inspection #3594

Conversation

frrist commented Oct 25, 2019

Motivation

anorth commented Oct 27, 2019

frrist commented Oct 28, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Oct 31, 2019 • edited

Codecov Report

Choose a reason for hiding this comment

ZenGround0 left a 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

frrist Nov 11, 2019 • edited

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

ZenGround0 left a comment

Choose a reason for hiding this comment

codecov-io commented Oct 31, 2019 •

edited

frrist Nov 11, 2019 •

edited