utreexo icon indicating copy to clipboard operation
utreexo copied to clipboard

accumulator/batchproof: Remove the numTargets max check in deserialize

Open kcalvinalvin opened this issue 3 years ago • 3 comments

In mainnet, the check for 1<<16 numTargets doesn't work because there's a mainnet block that crashes with this but doesn't with 1<<31. Not sure which block and exactly how many inputs it's referencing.

https://github.com/mit-dci/utreexo/blob/71712c452df5e77f9f593261ad0229765995dcac/accumulator/batchproof.go#L108-L111

But this is just a check to see if the proof was serialized correctly right? We should remove the whole check completely and replace it with some tests.

kcalvinalvin avatar Apr 17 '21 06:04 kcalvinalvin

It's weird because the numbers seem close to 65K, not like millions / billions, suggesting that numTargets might actually be correct.

But it can't be; numTargets should be at most the number of inputs in a block (less the skipped 0-conf inputs), and there can't be 65K inputs in a bitcoin block. They all have at least 36 byte (32 byte txid, 4 byte index) outpoints, meaning there can be at most 27777 targets in a block. And realistically much fewer.

So something else weird is going on, like too many targets being added, duplicate targets, or targets that don't correspond to inputs or something...

adiabat avatar Apr 26 '21 16:04 adiabat

It's weird because the numbers seem close to 65K, not like millions / billions, suggesting that numTargets might actually be correct.

But it can't be; numTargets should be at most the number of inputs in a block (less the skipped 0-conf inputs), and there can't be 65K inputs in a bitcoin block. They all have at least 36 byte (32 byte txid, 4 byte index) outpoints, meaning there can be at most 27777 targets in a block. And realistically much fewer.

So something else weird is going on, like too many targets being added, duplicate targets, or targets that don't correspond to inputs or something...

Might also just be the utcd code ugh...

kcalvinalvin avatar Apr 27 '21 06:04 kcalvinalvin

 func (bp *BatchProof) Serialize(w io.Writer) (err error) {
+       if len(bp.Proof) > 1<<16 {
+               err = fmt.Errorf("%d targets - too many\n", len(bp.Proof))
+               panic(err)
+       }

Panics when doing this (tested on commit 71712c452df5e77f9f593261ad0229765995dcac). We're likely re-adding targets or something.. :/

EDIT:

So in the code snippet below, we're adding extra data besides the proofs so it's possible to be greater than 1<<16. It's proof + sorted targets that we're putting in the proofs.

https://github.com/mit-dci/utreexo/blob/deb3c4cdd1ab7290f1e50eac99b6d7f0ee7d9425/accumulator/forestproofs.go#L172-L189

kcalvinalvin avatar Apr 28 '21 07:04 kcalvinalvin