Add the ipfs dag api object in Blockstore #356

evan-forbes · 2021-05-26T03:05:30Z

Description

This PR adds the ipfs object to the Blockstore struct, along with adding a method to get the IPFS dag api object to the BlockStore interface. It also removes it from the State and Node objects. It does not do anything new with the ipfs api object.

Piggy backing on the work of #352, this PR introduces DagOnlyMock. In this case, it was needed to get TestNodeSetPrivValIPC to pass.

There are also a few timeout increases in a meager attempt to decrease CI flakiness.

Closes: #355

…ject

codecov-commenter · 2021-05-27T03:29:32Z

Codecov Report

Merging #356 (2e5a7e4) into master (f46cbc6) will decrease coverage by 0.02%.
The diff coverage is 68.42%.

@@            Coverage Diff             @@
##           master     #356      +/-   ##
==========================================
- Coverage   61.81%   61.78%   -0.03%     
==========================================
  Files         262      262              
  Lines       22920    22920              
==========================================
- Hits        14167    14161       -6     
+ Misses       7259     7257       -2     
- Partials     1494     1502       +8

Impacted Files	Coverage Δ
consensus/replay_file.go	`0.00% <0.00%> (ø)`
state/services.go	`40.00% <ø> (ø)`
node/node.go	`56.33% <77.77%> (+0.38%)`	⬆️
store/store.go	`64.88% <80.00%> (-0.14%)`	⬇️
config/config.go	`78.24% <100.00%> (ø)`
consensus/state.go	`67.92% <100.00%> (+0.52%)`	⬆️
privval/signer_listener_endpoint.go	`80.00% <0.00%> (-9.42%)`	⬇️
p2p/switch.go	`63.27% <0.00%> (-4.63%)`	⬇️
privval/socket_listeners.go	`78.72% <0.00%> (-4.26%)`	⬇️
privval/secret_connection.go	`72.68% <0.00%> (-3.61%)`	⬇️
... and 8 more

evan-forbes · 2021-05-27T03:31:52Z

ipfs/mock.go

+// DagOnlyMock provides an empty APIProvider that only mocks the DAG portion of
+// the ipfs api object. This is much lighter than the full IPFS node and should
+// be favored for CI testing
+func DagOnlyMock() APIProvider {
+	mockAPI := dagOnlyMock{mdutils.Mock()}
+	return func() (coreiface.CoreAPI, io.Closer, error) {
+		return mockAPI, mockAPI, nil
+	}
+}
+
+var _ coreiface.CoreAPI = dagOnlyMock{}
+
+type dagOnlyMock struct {
+	format.DAGService
+}
+
+func (dom dagOnlyMock) Dag() coreiface.APIDagService { return dom }
+
+func (dagOnlyMock) Unixfs() coreiface.UnixfsAPI                                   { return nil }
+func (dagOnlyMock) Block() coreiface.BlockAPI                                     { return nil }
+func (dagOnlyMock) Name() coreiface.NameAPI                                       { return nil }
+func (dagOnlyMock) Key() coreiface.KeyAPI                                         { return nil }
+func (dagOnlyMock) Pin() coreiface.PinAPI                                         { return nil }
+func (dagOnlyMock) Object() coreiface.ObjectAPI                                   { return nil }
+func (dagOnlyMock) Dht() coreiface.DhtAPI                                         { return nil }
+func (dagOnlyMock) Swarm() coreiface.SwarmAPI                                     { return nil }
+func (dagOnlyMock) PubSub() coreiface.PubSubAPI                                   { return nil }
+func (dagOnlyMock) ResolvePath(context.Context, path.Path) (path.Resolved, error) { return nil, nil }
+func (dagOnlyMock) ResolveNode(context.Context, path.Path) (format.Node, error)   { return nil, nil }
+func (dagOnlyMock) WithOptions(...options.ApiOption) (coreiface.CoreAPI, error)   { return nil, nil }
+func (dagOnlyMock) Close() error                                                  { return nil }
+func (dagOnlyMock) Pinning() format.NodeAdder                                     { return nil }


I'm not the biggest fan of these nil mocks, but it does give us a lot flexibility in the future by not modifying APIProvider, which returns the full iface.CoreAPI. If we need the full ipfs API for whatever reason, then we can still use the normal Mock. If we don't, then we can use the much lighter dagOnlyMock

tbc, I'm not attached to this solution, and am very open to other ideas for passing in a dag only mock.

It's probably better to actually just pass around the DAG / the absolutely necessary API surface in these cases as well (like @Wondertan did in #352) but I think having different mocks for different purposes might still be useful. Especially as unit-tests most likely will rarely really need the whole CoreAPI object.
I'm OK with this. Would like to hear what @Wondertan thinks about this too.

Agree with @liamsi, we can simply deprecate full core API usage. We won't need anything form it except for the DAG.

So I don't see any need for dagOnlyMock

alrighty, the api object has been kicked from the island 1bddff2

evan-forbes · 2021-05-27T03:36:49Z

node/node_test.go

-		ipfs.Mock(),
+		ipfs.DagOnlyMock(),


The full mock node was part of the reason why TestNodeSetPrivValIPC kept failing in the CI (passes locally). TBH, I'm still not 100% sure why, as this test was passing in #352, and the mocked IPFS api obejct was only moved to the blockstore. 🤷‍♂️

evan-forbes · 2021-05-27T03:42:17Z

node/node.go

@@ -216,17 +215,20 @@ type Node struct {
 	indexerService    *txindex.IndexerService
 	prometheusSrv     *http.Server

-	ipfsAPI   ipface.CoreAPI


removed this for now, as we weren't using it anywhere. We can easily add it back in if needed.

liamsi · 2021-05-27T08:16:09Z

test-output2.txt

+ok  	github.com/lazyledger/lazyledger-core/types	(cached)
+?   	github.com/lazyledger/lazyledger-core/types/consts	[no test files]
+ok  	github.com/lazyledger/lazyledger-core/types/time	(cached)
+?   	github.com/lazyledger/lazyledger-core/version	[no test files]


This was probably checkedin by accident only?

whoops. Thanks for catching this! removed c11b9fe

Wondertan

Overall good but my concerns are:

Encapsulating DAG in the Blockstore
Keep relying on CoreAPI

Wondertan · 2021-05-27T17:05:15Z

store/store.go

@@ -424,6 +428,12 @@ func (bs *BlockStore) SaveSeenCommit(height int64, seenCommit *types.Commit) err
 	return bs.db.Set(calcSeenCommitKey(height), seenCommitBytes)
 }

+// IpfsAPI returns the ipfs api object of the BlockStore. Fullfills the
+// state.BlockStore interface.
+func (bs *BlockStore) IpfsDagAPI() format.DAGService {


What about only DAG() or IpfsDAG()? Api seems redundant to me here

Also, the comment does not correspond to the name

removed the entire method 8a4026c because we were no longer using it after fixing #356 (comment)

Wondertan · 2021-05-27T17:06:23Z

store/store.go

@@ -5,6 +5,7 @@ import (
 	"strconv"

 	"github.com/gogo/protobuf/proto"
+	format "github.com/ipfs/go-ipld-format"


Let's use ipld alias for go-ipld-format everywhere. I also did that in the previous PR

changed here b2469cc

left it as format in the consensus and light packages as they use our own ipld package

Yep, I am going to propose the rename of ipld package soon. We should change its name as it does not explain what it has inside

Wondertan · 2021-05-27T17:08:52Z

ipfs/mock.go

+// DagOnlyMock provides an empty APIProvider that only mocks the DAG portion of
+// the ipfs api object. This is much lighter than the full IPFS node and should
+// be favored for CI testing
+func DagOnlyMock() APIProvider {
+	mockAPI := dagOnlyMock{mdutils.Mock()}
+	return func() (coreiface.CoreAPI, io.Closer, error) {
+		return mockAPI, mockAPI, nil
+	}
+}
+
+var _ coreiface.CoreAPI = dagOnlyMock{}
+
+type dagOnlyMock struct {
+	format.DAGService
+}
+
+func (dom dagOnlyMock) Dag() coreiface.APIDagService { return dom }
+
+func (dagOnlyMock) Unixfs() coreiface.UnixfsAPI                                   { return nil }
+func (dagOnlyMock) Block() coreiface.BlockAPI                                     { return nil }
+func (dagOnlyMock) Name() coreiface.NameAPI                                       { return nil }
+func (dagOnlyMock) Key() coreiface.KeyAPI                                         { return nil }
+func (dagOnlyMock) Pin() coreiface.PinAPI                                         { return nil }
+func (dagOnlyMock) Object() coreiface.ObjectAPI                                   { return nil }
+func (dagOnlyMock) Dht() coreiface.DhtAPI                                         { return nil }
+func (dagOnlyMock) Swarm() coreiface.SwarmAPI                                     { return nil }
+func (dagOnlyMock) PubSub() coreiface.PubSubAPI                                   { return nil }
+func (dagOnlyMock) ResolvePath(context.Context, path.Path) (path.Resolved, error) { return nil, nil }
+func (dagOnlyMock) ResolveNode(context.Context, path.Path) (format.Node, error)   { return nil, nil }
+func (dagOnlyMock) WithOptions(...options.ApiOption) (coreiface.CoreAPI, error)   { return nil, nil }
+func (dagOnlyMock) Close() error                                                  { return nil }
+func (dagOnlyMock) Pinning() format.NodeAdder                                     { return nil }


Agree with @liamsi, we can simply deprecate full core API usage. We won't need anything form it except for the DAG.

Wondertan · 2021-05-27T17:09:40Z

ipfs/mock.go

+// DagOnlyMock provides an empty APIProvider that only mocks the DAG portion of
+// the ipfs api object. This is much lighter than the full IPFS node and should
+// be favored for CI testing
+func DagOnlyMock() APIProvider {
+	mockAPI := dagOnlyMock{mdutils.Mock()}
+	return func() (coreiface.CoreAPI, io.Closer, error) {
+		return mockAPI, mockAPI, nil
+	}
+}
+
+var _ coreiface.CoreAPI = dagOnlyMock{}
+
+type dagOnlyMock struct {
+	format.DAGService
+}
+
+func (dom dagOnlyMock) Dag() coreiface.APIDagService { return dom }
+
+func (dagOnlyMock) Unixfs() coreiface.UnixfsAPI                                   { return nil }
+func (dagOnlyMock) Block() coreiface.BlockAPI                                     { return nil }
+func (dagOnlyMock) Name() coreiface.NameAPI                                       { return nil }
+func (dagOnlyMock) Key() coreiface.KeyAPI                                         { return nil }
+func (dagOnlyMock) Pin() coreiface.PinAPI                                         { return nil }
+func (dagOnlyMock) Object() coreiface.ObjectAPI                                   { return nil }
+func (dagOnlyMock) Dht() coreiface.DhtAPI                                         { return nil }
+func (dagOnlyMock) Swarm() coreiface.SwarmAPI                                     { return nil }
+func (dagOnlyMock) PubSub() coreiface.PubSubAPI                                   { return nil }
+func (dagOnlyMock) ResolvePath(context.Context, path.Path) (path.Resolved, error) { return nil, nil }
+func (dagOnlyMock) ResolveNode(context.Context, path.Path) (format.Node, error)   { return nil, nil }
+func (dagOnlyMock) WithOptions(...options.ApiOption) (coreiface.CoreAPI, error)   { return nil, nil }
+func (dagOnlyMock) Close() error                                                  { return nil }
+func (dagOnlyMock) Pinning() format.NodeAdder                                     { return nil }


So I don't see any need for dagOnlyMock

Wondertan · 2021-05-27T17:13:57Z

consensus/state.go

@@ -94,8 +93,6 @@ type State struct {
 	// store blocks and commits
 	blockStore sm.BlockStore

-	dag format.DAGService


Pls, keep DAGService in the State how it was. DAGService handles networking as well and making State rely on dag service provided by blockstore is architecturally not a good idea.

Conceptually, I argue that we should change "move the ipfs dag api object to Blockstore" to "use the ipfs dag api object in Blockstore"

that is now true, yes.

…consensus state

Wondertan · 2021-05-27T19:07:07Z

ipfs/provider.go

@@ -7,4 +7,4 @@ import (
 )

 // APIProvider allows customizable IPFS core APIs.
-type APIProvider func() (coreiface.CoreAPI, io.Closer, error)
+type APIProvider func() (coreiface.APIDagService, io.Closer, error)


👍🏻
This should make the IPFS resource usage footprint in tests very low!

liamsi

You guys rock 🚀 Great work and thanks for the thorough review as well @Wondertan.

Wondertan · 2021-05-27T21:38:12Z

The further we go, the more we disassemble the IPFS blob. I start to like this approach as it appears to be more natural than just going straight into breaking it.

… of embedded ipfs objects This reverts commit 40acb17.

* Revert "Remove the PartSetHeader from VoteSetMaj23, VoteSetBits, and state.State (#479)" This reverts commit 3ba47c2. * Revert "Stop Signing over the `PartSetHeader` for Votes and remove it from the `Header` (#457)" This reverts commit 818be04. * Revert "Decouple PartSetHeader from BlockID (#441)" This reverts commit 9d4265d. * Revert "Save and load block data using IPFS (#374)" This reverts commit 8da1644. * fix merge error * Revert "Add the ipfs dag api object in Blockstore (#356)" and get rid of embedded ipfs objects This reverts commit 40acb17. * remove ipfs node provider from new node * stop init ipfs repos * remove IPFS node config * clean up remaining ipfs stuff

add the ipfs object to the blockstore and remove if from the state ob…

aabfd03

…ject

evan-forbes requested review from liamsi and Wondertan May 26, 2021 03:05

evan-forbes self-assigned this May 26, 2021

evan-forbes requested a review from tac0turtle as a code owner May 26, 2021 03:05

evan-forbes removed the request for review from tac0turtle May 26, 2021 03:05

evan-forbes added 4 commits May 25, 2021 22:09

fix linter's complaints

5c63080

increase TestReactorSelectiveBroadcast sleep times

f0a8a34

increase ensureTimeout to 4 seconds

e32ce9d

only use the dag api object instead of the entire ipfs api object

0c06595

evan-forbes changed the title ~~Move the ipfs api object to Blockstore~~ Move the ipfs dag api object to Blockstore May 26, 2021

evan-forbes added 14 commits May 26, 2021 14:08

increase TestNodeSetPrivValIPC timeout to 400ms

0517ac2

Merge branch 'master' into evan/move-ipfs-to-blockstore

16a9230

increase time waited for TestReactorsGossipNoCommittedEvidence again

ac48a99

increase TestNodeSetPrivValIPC timeout again

87fae07

timeout increase

eed5e89

cleanup remainging mocks

8d5debe

try insane timeout for TestNodeSetPrivValIPC

6174063

increase the failing precommit timeout

fbfab50

more cleanup

2bb033b

remove the unused ipfsAPI from the node

210e690

try a test node that doesn't use the full mocked ipfs node

e914706

implement and use a dag only api provider

cb645cb

revert crazy timeout

00d3af0

simplify dag only mock

2e5a7e4

evan-forbes commented May 27, 2021

View reviewed changes

evan-forbes mentioned this pull request May 27, 2021

Move the IPFS API Object to the BlockStore #355

Closed

liamsi reviewed May 27, 2021

View reviewed changes

evan-forbes added 2 commits May 27, 2021 07:38

remove accidental file

c11b9fe

try to make TestReactorsGossipNoCommittedEvidence less flaky

e55d925

Wondertan requested changes May 27, 2021

View reviewed changes

evan-forbes added 4 commits May 27, 2021 12:58

use ipld alias instead of format

b2469cc

remove access to the IPFS dag from the blockstore and add it back to …

8a4026c

…consensus state

change api provider to only use the dag instead of the core api object

1bddff2

change alias to ipld in node package

71bcef7

Wondertan approved these changes May 27, 2021

View reviewed changes

increase timeouts for TestWALTruncate and timeoutWaitGroup for CI

0ce8cec

evan-forbes changed the title ~~Move the ipfs dag api object to Blockstore~~ Use the ipfs dag api object in Blockstore May 27, 2021

evan-forbes changed the title ~~Use the ipfs dag api object in Blockstore~~ Add the ipfs dag api object in Blockstore May 27, 2021

liamsi approved these changes May 27, 2021

View reviewed changes

evan-forbes merged commit 40acb17 into master May 27, 2021

evan-forbes deleted the evan/move-ipfs-to-blockstore branch May 27, 2021 22:08

evan-forbes mentioned this pull request May 28, 2021

Save and load block data using IPFS #374

Merged

evan-forbes added a commit that referenced this pull request Aug 25, 2021

Revert "Add the ipfs dag api object in Blockstore (#356)" and get rid…

225ee8b

… of embedded ipfs objects This reverts commit 40acb17.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add the ipfs dag api object in Blockstore #356

Add the ipfs dag api object in Blockstore #356

evan-forbes commented May 26, 2021 •

edited

Loading

codecov-commenter commented May 27, 2021 •

edited

Loading

evan-forbes May 27, 2021 •

edited

Loading

evan-forbes May 27, 2021 •

edited

Loading

liamsi May 27, 2021

Wondertan May 27, 2021

Wondertan May 27, 2021

evan-forbes May 27, 2021

evan-forbes May 27, 2021 •

edited

Loading

evan-forbes May 27, 2021

liamsi May 27, 2021

evan-forbes May 27, 2021 •

edited

Loading

Wondertan left a comment

Wondertan May 27, 2021

evan-forbes May 27, 2021

Wondertan May 27, 2021

evan-forbes May 27, 2021

Wondertan May 27, 2021

Wondertan May 27, 2021

Wondertan May 27, 2021

Wondertan May 27, 2021

Wondertan May 27, 2021 •

edited

Loading

evan-forbes May 27, 2021

Wondertan May 27, 2021

evan-forbes May 27, 2021

liamsi left a comment

Wondertan commented May 27, 2021

Add the ipfs dag api object in Blockstore #356

Add the ipfs dag api object in Blockstore #356

Conversation

evan-forbes commented May 26, 2021 • edited Loading

Description

codecov-commenter commented May 27, 2021 • edited Loading

Codecov Report

evan-forbes May 27, 2021 • edited Loading

Choose a reason for hiding this comment

evan-forbes May 27, 2021 • edited Loading

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

evan-forbes May 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evan-forbes May 27, 2021 • edited Loading

Choose a reason for hiding this comment

Wondertan 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

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

Wondertan May 27, 2021 • edited Loading

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

liamsi left a comment

Choose a reason for hiding this comment

Wondertan commented May 27, 2021

evan-forbes commented May 26, 2021 •

edited

Loading

codecov-commenter commented May 27, 2021 •

edited

Loading

evan-forbes May 27, 2021 •

edited

Loading

evan-forbes May 27, 2021 •

edited

Loading

evan-forbes May 27, 2021 •

edited

Loading

evan-forbes May 27, 2021 •

edited

Loading

Wondertan May 27, 2021 •

edited

Loading