refactor: Use slices instead of maps #188

dergoegge · 2020-08-21T15:25:43Z

was part of my experiments in #180.
Refactor ProveBatch, verifyBatchProof and IngestBatchProof to use slices.
Preallocation of slices also helps with GC pressure.
also includes the ProofPositions function which is also part of #185, kinda stupid but otherwise this doesn't build.

accumulator/utils.go

dergoegge · 2020-08-21T15:30:30Z

accumulator/pollardproof.go

+		return fmt.Errorf("block proof mismatch")
+	}
+	// preallocating polNodes helps with garbage collection
+	polNodes := make([]polNode, len(trees)*3)


preallocating the polNodes before populating the pollard was the most significant improvement to gc pressure.
my assumption is that this helps because the polNodes are next to each other in memory so it's easier for the gc to do its job.

accumulator/pollardproof.go

accumulator/batchproof.go

kcalvinalvin · 2020-08-22T07:14:19Z

accumulator/utils.go

+	// the positions that are computed/not included in the proof. (also includes the targets)
+	computedPositions := make([]uint64, 0, len(targets)*int(forestRows))
+	for row := uint8(0); row < forestRows; row++ {
+		computedPositions = append(computedPositions, targets...)


This could be moved outside the loop right?

oh nvm targets are mutated.

EDIT: No wait so all the targets are already allocated in the first go. All the ones afterwards just are duplicates.

Also seems like the computed positions are only used for pre-allocating in verifyBatchProof in batchproof.go so this could be moved outside the loop I think. Would also help in further lessening the gc pressure.

I don't think it can. I think my naming here (targets, nextTargets) is a bit off so that might be why it seems wrong. On every iteration targets holds the positions of the nodes on that row that are known, for row=0 it's the actual targets contained in the proof, on the rows above it's the the positions that can be computed. The nextTargets variable is used to store the positions on the next row that can be computed and at the end of each iteration targets is assigned to nextTargets.

Also seems like the computed positions are only used for pre-allocating in verifyBatchProof in batchproof.go so this could be moved outside the loop I think. Would also help in further lessening the gc pressure.

the computed positions are also needed in #185, but yeah in verifyBatchProof they are unnecessary

accumulator/batchproof.go

kcalvinalvin · 2020-08-28T05:40:15Z

accumulator/batchproof.go

-	// and has led to a number of bugs.  It *is* special in a way, in that
-	// the bottom row is the only thing you actually prove and add/delete,
-	// but it'd be nice if it could all be treated uniformly.
+		if len(bp.Proof) < 2 || len(targets) < 2 {


What would be a case where this clause is true?

i added a comment there, let me know if that explains it.

kcalvinalvin · 2020-08-28T05:44:46Z

accumulator/batchproof.go

+			rootMatches++
+		}
+	}
+	if len(rootCandidates) != rootMatches {


Since all roots should match in order for the batchProof to be valid, wouldn't it be better to return false in the block at line 232 if any root doesn't match a rootCandidate?

rootCandidates only holds a subset of the roots, so it could happen that a root does not match a candidate but by making sure that all candidates were matched to some root it still works out.
also added a comment there.

accumulator/pollardproof.go

kcalvinalvin · 2020-09-17T16:48:46Z

Is there any code that would cause backwards incompatibility with master? Right now, It's fine when I point the csn to my local bridgenode based off of this PR but crashes with this message when I point it at the current default server.

[N] calvin@bitcoin ~/b/u/g/s/g/m/u/c/utreexoclient> ./utreexoclient -checksig=0
Creating new pollarddata
Verify failed: txo 6749762ae220c10705556799dcec9bb6a54a7b881eb4b961323a3363b00db518:0 position 103 leafdata 7b9562ebac55529d473d4d0bb616ee5d9f69898b1cc72051c204321666abea6d proof b628e99d6a24cbb09b35253756439ba827d27bc0b20c0d02a7bed7fc1dae4287
sib exists, af6aa1d9405b73e596cb3251c357f7dfdbbbece7f2db02176440098482bd64a2
panic: height 381 LeafData / Proof mismatch

goroutine 9 [running]:
github.com/mit-dci/utreexo/csn.(*Csn).IBDThread(0xc00000c1e0, 0xc000034150)
        /home/calvin/bitcoin-projects/utreexo1/go/src/github.com/mit-dci/utreexo/csn/ibd.go:56 +0xa29
created by github.com/mit-dci/utreexo/csn.(*Csn).Start
        /home/calvin/bitcoin-projects/utreexo1/go/src/github.com/mit-dci/utreexo/csn/init.go:101 +0x243

dergoegge · 2020-09-17T19:17:54Z

Is there any code that would cause backwards incompatibility with master?

That's a great test. The roots should be exactly the same for every block.

I think it errors because on master right now the 0-tree root is being send.

utreexo/accumulator/forestproofs.go

Lines 195 to 201 in 7f5d091

    
           // TODO change this for the real thing; no need to prove 0-tree root. 
        
           // but we still need to verify it and tag it as a target. 
        
           if pos == f.numLeaves-1 && pos&1 == 0 { 
        
           	proofTree[pos] = f.data.read(pos) 
        
           	// fmt.Printf("%d add as root\n", pos) 
        
           	continue 
        
           }

~~In this PR however those are not send/expected by the csn. I will check if that is actually the problem here.~~

Edit: Actually the 0-tree roots are still being send. This is something else.

dergoegge commented Aug 21, 2020

View reviewed changes

dergoegge mentioned this pull request Aug 21, 2020

[WIP] caching #180

Closed

kcalvinalvin reviewed Aug 23, 2020

View reviewed changes

dergoegge force-pushed the useslices branch from b6411f1 to 7f63247 Compare August 24, 2020 10:47

kcalvinalvin reviewed Aug 28, 2020

View reviewed changes

accumulator/batchproof.go Outdated Show resolved Hide resolved

kcalvinalvin reviewed Aug 28, 2020

View reviewed changes

accumulator/batchproof.go Show resolved Hide resolved

kcalvinalvin reviewed Aug 28, 2020

View reviewed changes

accumulator/batchproof.go Show resolved Hide resolved

kcalvinalvin reviewed Aug 28, 2020

View reviewed changes

accumulator/pollardproof.go Show resolved Hide resolved

dergoegge force-pushed the useslices branch 2 times, most recently from 96cf573 to 06336b8 Compare September 5, 2020 19:02

refactor: Use slices instead of maps

3c1e23e

dergoegge force-pushed the useslices branch from 06336b8 to 3c1e23e Compare September 8, 2020 14:34

adiabat merged commit b3afd5d into mit-dci:master Sep 22, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Use slices instead of maps #188

refactor: Use slices instead of maps #188

dergoegge commented Aug 21, 2020

dergoegge Aug 21, 2020

kcalvinalvin Aug 22, 2020

kcalvinalvin Aug 22, 2020

dergoegge Aug 23, 2020

dergoegge Aug 23, 2020

kcalvinalvin Aug 28, 2020

dergoegge Sep 5, 2020

kcalvinalvin Aug 28, 2020

dergoegge Sep 5, 2020

kcalvinalvin commented Sep 17, 2020

dergoegge commented Sep 17, 2020 •

edited

Loading

refactor: Use slices instead of maps #188

refactor: Use slices instead of maps #188

Conversation

dergoegge commented Aug 21, 2020

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

kcalvinalvin commented Sep 17, 2020

dergoegge commented Sep 17, 2020 • edited Loading

dergoegge commented Sep 17, 2020 •

edited

Loading