add transparent data to compact block serialization#534
add transparent data to compact block serialization#534nuttycom merged 8 commits intozcash:masterfrom
Conversation
|
On mainnet, to recreate the entire compact block cache (resync) took about 8.5 hours on my mid-level desktop, using zebrad as the backend. The cache size is 33 GB, about double the pre-this-PR size. This is expected for two reasons: first, we're now storing some parts of every transparent transaction, instead of (parts of) only shielded transactions. Secondly, we're storing blocks between height 0 and 419200 (mainnet sapling activation height). UPDATE: I should also mention that the way this is implemented, the |
This close to what the statistical analysis indicated, but we didn't run it since genesis because we didn't consider that use case of people importing a pre-sapling wallet into a shielded light-client |
I asked @nuttycom and he said the cache should go back to genesis. It's not much more space (I didn't calculate how much), so I think it's okay to leave it even if it might not be needed. |
1a2759d to
427ccbd
Compare
pacu
left a comment
There was a problem hiding this comment.
utACK modulo documentation suggestions
| repeated CompactSaplingSpend spends = 4; | ||
| repeated CompactSaplingOutput outputs = 5; | ||
| repeated CompactOrchardAction actions = 6; | ||
| // CompactTxIn values corresponding to the `vin` entries of the full transaction. |
There was a problem hiding this comment.
| // CompactTxIn values corresponding to the `vin` entries of the full transaction. | |
| // CompactTxIn values corresponding to the `vin` entries of the full transaction. |
nuttycom
left a comment
There was a problem hiding this comment.
Reviewed 427ccbd
Overall this looks good, but I think there are problems with a number of the transaction version checks in parsing in that I don't believe that they function correctly for V5 transactions as currently written.
CHANGELOG.md
Outdated
|
|
||
| ### Added | ||
|
|
||
| - Add block header to GetBlock and GetBlockRange output (issue 527) |
There was a problem hiding this comment.
This should mention the .proto update (once it has been brought in via subtree merge and once we've sorted out the tagging on the zcash/lightwallet-protocol repo.)
There was a problem hiding this comment.
How is this test data generated? It would be useful to document this (though maybe it's documented elsewhere, since .json files can't contain comments?).
There was a problem hiding this comment.
How is this test data generated?
This data dates back to the earliest versions of the project, when George Tankersley maintained it, so I'm unsure. The more recent changes that I've made were sort of cheating; we don't have an independent way to generate compact blocks and transactions, so I just let the code tell me what it should be. This does at least catch regressions — if the tests that use this data start failing when we don't expect them to, then we know something has inadvertently broken in either the deserialization of the full block, conversion to its compact format, or in the serialization of the compact block.
LarryRuane
left a comment
There was a problem hiding this comment.
I left comments on most of your comments, I'll push changes, thanks!
parser/transaction.go
Outdated
|
|
||
| if !s.Skip(192) { | ||
| return nil, errors.New("could not skip Groth16 proof") | ||
| if isSaplingV4 { |
There was a problem hiding this comment.
Yes, I think you're correct. There must not be any joinSplits after V5 activated on mainnet, because the entire chain parses correctly. But this must be fixed (for the future). One test we definitely should do (and I will) is verify that lightwalletd parses testnet correctly; there may be transaction formats there that don't exist on mainnet.
We may still want to introduce the enum, but for now, I'll add a boolean that indicates if the joinsplit proof is Groth16 (Sapling or later) or not.
parser/transaction.go
Outdated
| if !s.ReadBytes(&b32, 32) { | ||
| return nil, errors.New("could not read HashPrevBlock") | ||
| } | ||
| tx.Outpoint.PrevTxHash = hash32.T(b32) |
There was a problem hiding this comment.
I think it should be FromSlice, will fix it.
427ccbd to
4185b7c
Compare
4185b7c to
151268a
Compare
|
Force pushed to
I think this is ready to merge, except I'm still unclear on the proto files with respect to the separate repo (git subtree), so this PR probably isn't mergeable exactly as is. Working on these changes also led to creating zcash/lightwallet-protocol#5 (documentation only change). |
|
@LarryRuane the way to handle updating the protobuf files is just to do the following: I would create a temporary branch from |
151268a to
1f4eb17
Compare
Thanks, did that, force pushed, please review. Ready to merge, AFAIK. |
1f4eb17 to
2b6c547
Compare
|
To check for regressions, I ran the following command against this PR and again against the This fetches all blocks between Sapling activation and (near) the current chain tip. The results are identical. This shows that the following actions are correct for each block using the PR branch (or at least that the PR doesn't introduce any new bugs):
This doesn't verify transparent transaction elements, because the point of the test is to compare the PR with This was with the local I did the same test on testnet (since there may be some unusual transaction formats that aren't on mainnet), and the results were also identical. |
37e8ca7 to
5f48fc4
Compare
|
Rebased, and also fix commit "add header to GetBlock result (issue 527) if requested" by adding the "requested" part: don't unconditionally include the full block header in the compact blocks returned by AFAIK, this is ready to be merged (no other changes planned), just needs review. |
|
me:
Correction -- there are still a few things left to address, don't merge yet. |
5f48fc4 to
401e540
Compare
|
Rebased onto the latest |
Shouldn't this be just based on the released 0.4.0 tag? We don't want to end up releasing a lightwalletd that's based on a version of the lightwallet-protocol API that is not tagged as a release, and I don't think that the delta between 0.4.0 and the unreleased version that this is based on is meaningful. |
The main work is adding the ability to parse pre-V4 (Sapling) transactions. I used zcash: src/primitives/transaction.h to see how the serialization is done.
1d59069 to
a41a2de
Compare
That's a good point. I just force pushed to remove the subtree commit, since |
a41a2de to
d2de8f5
Compare
|
Another force push (d2de8f5) to address all remaining review comments; I think this is finally ready to merge (after re-review). |
frontend/service.go
Outdated
| return status.Errorf(codes.InvalidArgument, "invalid pool type requested") | ||
| } | ||
| if len(exclude.PoolTypes) == 0 { | ||
| // legacy behavior: return only blocks containing sheilded components. |
There was a problem hiding this comment.
This was marked as resolved but has not been resolved.
This is a space and network optimization; the coinbase input doesn't contain relevant information for us (our compact block inputs are just an outpoint, which is a txid and index, and for the coinbase, these aren't meaningful).
No functional difference, just clarity. Change calls such as: hash32.T(someslice) to: hash32.FromSlice(someslice)
The first time a release with this PR runs, it will find a compact block disk cache that doesn't begin at height zero, doesn't include transparent transaction components, and doesn't contain the full block header. The way this will be fixed is to re-create the cache from scratch. To detect that this needs to be done, we attempt to deserialize (only) the first block. If that fails, rebuild the cache. As part of this, simplify the recovery when disk cache corruption is detected. Instead of backing up 10,000 blocks and trying again (which caused an unintentional function call sequence recursion), rebuild the entire cache. If there is any corruption detected, it's best to rebuild the entire cache, rather than trying to optimize and rebuild only part of it.
This uses the recently-added BlockRange.poolTypes argument; previously, GetBlockRange ignored this argument.
The PoolTypes function argument is already in service.proto. So the change in behavior here is that instead of GetMempoolTx returning all transactions (and all parts of those transactions), ignoring the exclude.PoolType argument; now it will (1) return only the requested parts of transactions (rather than the entire transaction), and (2) only return transactions that have at least one part after pool filtering. This is the same behavior as GetBlockRange.
d2de8f5 to
1e58b2a
Compare
|
Addressed all review comments, thanks @str4d |
This PR includes changing the proto files directly, which is incorrect and is actually being done by zcash/lightwallet-protocol#1, so these changes will have to be removed before merging. There is also probably some documentation that should be added / changed here. But it's in a good enough state to be reviewed and tested now.
This PR does not provide the value of each transparent input, as suggested as a possible enhancement in the PR referenced above. It will probably increase the cache build time too much. But I think I'll implement as (potentially) part of this PR, and see if that's the case. It is pretty easy to code.