Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

protocol or blockformat changes? #991

Closed
jl777 opened this issue Jun 3, 2016 · 23 comments
Closed

protocol or blockformat changes? #991

jl777 opened this issue Jun 3, 2016 · 23 comments
Labels
A-consensus Area: Consensus rules A-documentation Area: Documentation protocol spec question

Comments

@jl777
Copy link

jl777 commented Jun 3, 2016

page 9 of https://github.com/zcash/zips/blob/master/protocol/protocol.pdf describes the JoinSplit fields that the transactions have. I assume that at least a single byte 0 field will always be present in case there are no JoinSplits, and N JoinSplits if it is non-zero.

Just to confirm, that this extension is at the end of the normal transaction. I am in the process of adding zcash support to iguanacore and it is already establishing contact with the testnet peers, but not able to properly parse the data. Are ANY other changes to the bitcoin protocol?

curl --url "http://127.0.0.1:7778" --data "{"RELAY":1,"VALIDATE":1,"prefetchlag":-1,"poll":10,"active":1,"agent":"iguana","method":"addcoin","startpend":8,"endpend":8,"services":129,"maxpeers":256,"newcoin":"ZEC","name":"Zcash","hasheaders":1,"useaddmultisig":0,"netmagic":"6df6e755","p2p":18333,"rpc":18332,"pubval":111,"p2shval":196,"wifval":239,"txfee_satoshis":"10000","isPoS":0,"minoutput":10000,"minconfirms":2,"genesishash":"27d1f4ce03fc473c9dd6e1e307c682c8f802eae1f5a2f61402aa1ae8702ed3b6","protover":70002,"genesisblock":"0100000000000000000000000000000000000000000000000000000000000000000000003ba3edfd7a7b12b27ac72c3e67768f617fc81bc3888a51323a9fb8aa4b1e5e4adae5494dffff7f2000000000"}"

The above API call to iguana starts the sync and I believe I got all the parameters extracted from the source code. Once the blocks can be parsed, then it will be possible to have block explorer level data available. Of course, I still need to figure out how to deal with the protected balances.

Any help appreciated

@ebfull
Copy link
Contributor

ebfull commented Jun 3, 2016

It is also the case that only version 2 transactions will have those additional fields at the end. Version 1 transactions are parsed exactly the same as Bitcoin. In that case you're looking at a version 1 transaction.

@str4d
Copy link
Contributor

str4d commented Jun 3, 2016

The proof of work we are using has necessitated a change to the block header format (see fdda3c5). You will need to account for the change in nonce size and the addition of an Equihash solution vector (of length 2^k with k = 5 currently for testnet). The block header will also change again when #724 lands.

@jl777
Copy link
Author

jl777 commented Jun 3, 2016

thanks! this will help a lot

@daira
Copy link
Contributor

daira commented Jun 3, 2016

page 9 of https://github.com/zcash/zips/blob/master/protocol/protocol.pdf describes the JoinSplit fields that the transactions have. I assume that at least a single byte 0 field will always be present in case there are no JoinSplits, and N JoinSplits if it is non-zero.

That's correct. The count field nJoinSplit is of compactSize uint type, so just a single byte when there are <= 252 JoinSplits. The spec hasn't yet been updated for the change in header format.

@daira
Copy link
Contributor

daira commented Jun 3, 2016

Another difference from the spec is that the JoinSplit proof (zkproof) is using a length field and text serialization, so is much longer than the 288 bytes in the spec. There's also an unexplained difference in proof length between the Zerocash paper and libsnark's binary serialization: zcash/zips#43

@daira daira added question A-documentation Area: Documentation A-consensus Area: Consensus rules protocol spec labels Jun 3, 2016
@ebfull
Copy link
Contributor

ebfull commented Jun 3, 2016

Right now the zkproof is not using text serialization and is currently ZKSNARK_PROOF_SIZE bytes long.

@jl777
Copy link
Author

jl777 commented Jun 4, 2016

It is very important from a performance point to have fixed size blockheaders. I am assuming 2^k with k = 5 means std::vector<uint32_t> nSolution in the header will have 32 uint32_t.

struct iguana_msgblockhdr_zcash
{
    uint32_t version;
    bits256 prev_block,merkle_root;
    uint32_t timestamp,bits;
    bits256 nonce;
    uint32_t solution[32];
    bits256 reserved; // only here if auxpow is set
} __attribute__((packed));

The above is the hardcoded blockheader for testnet with the k=5, will test this to see if there is another (byte/int) at the beginning of the solution for the number of solution ints. would be nice if it wasnt needed, maybe the k value can be encoded into the unused version bits

@str4d
Copy link
Contributor

str4d commented Jun 4, 2016

The block headers are fixed-size inasmuch as the Equihash parameters are fixed. We have already decided not to explicitly include the value of k, or other Equihash-specific version information, in the block header. Any changes to those will occur with a hardfork, which would be necessary to update the consensus for expected Equihash parameters even if they were included in the block header. Note that we can't just have the consensus allow parameters that result in higher memory usage than target, both for complexity (there is no linear relationship between the parameters and the memory usage), and because that could open up the proof-of-work to a vulnerability (if one person could find a drastic optimisation for a particular set of parameters that everyone else thinks requires higher-memory than the "minimum parameters").

But that only affects the interpretation of the block header. The solution in the block header is a serialized std::vector, which includes a compactSize uint length field preceding the entries. Thus parsers can just read the vector length and parse that many uint_32ts, and not need contextual hardfork information to know the actual value of k at that block height. Put another way, the length of the solution vector itself indirectly encodes k.

If you require a fixed-size block header for performance, simply parse and ignore the compactSize uint, and switch your block header format on block height.

Also note that the exact position of the reserved field is undecided yet.

@jl777
Copy link
Author

jl777 commented Jun 4, 2016

makes sense. but it seems that each hardfork will have a fixed number of vector elements. current network needs 1 byte for the 32 serialized uint32_t's, so 129 bytes of fixed size. I will just update my code with each hardfork as I assume there wont be that many of them once this is in production

I am a bit confused by the need for a compact size uint preceding the entries since you say "Any changes to those will occur with a hardfork", so it seems redundant and could be removed. Maybe you are planning for some diff adjustment to change the k value, or at least having the provision for that.

Couldnt you have a consensus method to increase k if the blocktimes get too fast? or maybe if the nbits value gets too low from the current diff readjustment. of course incrementing the k value is probably very non-linear and would be hard to do a matching nbits adjustment, especially if diff recalcs are once per 2016 blocks. Havent checked how frequently you are retargeting, if it is a smaller timeframe, maybe it isnt so bad to be non-linear as the nbits would get adjusted at the new k value

hardforks in the field are a giant pain, so best to make things as adaptive as possible, but I speak without any feel for how the k value changes time to solve, so maybe this isnt practical

@jl777
Copy link
Author

jl777 commented Jun 4, 2016

there is another discrepancy in the joinsplit docs. it says it is 1026 bytes in size, but I only add up to 1009:

struct iguana_msgjoinsplit
{
    uint64_t vpub_old,vpub_new;
    bits256 anchor,nullifiers,commitments,ephemeralkey;
    uint8_t ciphertexts[217];
    bits256 randomseed,vmacs;
    uint8_t zkproof[ZCASH_ZKPROOF_SIZE]; // 584
} __attribute__((packed));

2 * uint64_t + 6 * bits256 + 217 + 584 = 1009
Also since the format will have to change I strongly recomment to put the strange sized cipertexts at the end of the structure. This will allow aligned access to all the ints and bits256 for much faster memory accesses. Something like:

struct proposed_iguana_msgjoinsplit
{
    bits256 anchor,nullifiers,commitments,ephemeralkey,randomseed,vmacs;
    uint64_t vpub_old,vpub_new;
    uint8_t ciphertexts[217];
    uint8_t zkproof[]; // assume will become variable size
} __attribute__((packed));

by having all the bits256 in the beginning, it would also allow some array access to make the smaller codesize. iguana uses memory mapped data structures, so it is quite important how the fields line up

@ebfull
Copy link
Contributor

ebfull commented Jun 4, 2016

I think it's a good idea to put the ciphertexts at the end, what do you think @daira?

@jl777
Copy link
Author

jl777 commented Jun 4, 2016

if zkproof is variable length, then it should be the last one as variable size field can only be at the end of the structure

@ebfull
Copy link
Contributor

ebfull commented Jun 4, 2016

zkproof will be static length. It's supposed to be 288 bytes but we haven't finished.

@jl777
Copy link
Author

jl777 commented Jun 4, 2016

got it parsing the blocks (and I think tx), but it seems the blockheaders being sent back are not exactly the same. I will dig into it, but if anybody knows the difference, help appreciated

@jl777
Copy link
Author

jl777 commented Jun 4, 2016

it seems there are blocks with version 4 and also version 1. it is parsing the version 4, but getting out of sync after the version 1 block. I will try treating version 1 as original 80 header

@zookozcash
Copy link

@jl777: I just have to say that I'm so glad someone is digging into the nuts and bolts here and finding discrepancies or unexplained design decisions. We need this. Thank you!

@jl777
Copy link
Author

jl777 commented Jun 4, 2016

happy to help out.

iguana is now parallel syncing and parsing all blocks and tx, except for the blocks with more than 1 tx. I had to disable headers though, as the data seems to be a bit non-standard. pretty sure the problem with txhandling has to do with mismatched joinsplit. after that, I will need to figure out how to calculate balances. I am thinking that for addresses not in the wallet, all we can see is total balance, but not sure. hopefully there is some guidance on what sort of balances can be calculated and how to do it.

Once I get that I can make a special API to display it and then route this remotely

@jl777
Copy link
Author

jl777 commented Jun 4, 2016

Got it to parse.. But it is off by one byte in size:

struct iguana_msgjoinsplit
{
    uint64_t vpub_old,vpub_new;
    bits256 anchor,nullifiers[2],commitments[2],ephemeralkey;
    uint8_t ciphertexts[2][217];
    bits256 randomseed,vmacs[2];
    uint8_t zkproof[ZKSNARK_PROOF_SIZE-1];
} __attribute__((packed));

The above is what I had to make the joinsplit data and not sure where it is off by one, as I am not doing anything with the data other than parsing it, so it could be in any of the fields. It wasnt clear to me that there are two of nullifiers, commitments, cipertexts and vmacs. But after doubling up those fields things got to off by one.

Another strange thing that I see is that the transaction with the joinsplit has no normal output. Maybe that is expected, or maybe the source of the off by one. anyway, took all day, but it is now syncing to the latest block in about a minute, but then complaining about balances being off. To go further I need a bit more help, especially to know how to calculate address balances.

Thanks

@daira
Copy link
Contributor

daira commented Jun 4, 2016

+1 on moving the ciphertexts to the end.

@daira
Copy link
Contributor

daira commented Jun 4, 2016

And yes, it is really valuable for someone to implement this parsing independently. Thankyou.

@daira
Copy link
Contributor

daira commented Jun 22, 2016

Let's move the ciphertexts to the end if/when we figure out why the proof size is not 288 bytes. (It currently looks as though each element of the proof is occupying 2 bytes more than expected.)

@daira daira added this to the Consensus Code Security Freeze milestone Jun 22, 2016
@nathan-at-least nathan-at-least modified the milestones: Consensus Code Security Freeze, Temporary overflow Jul 14, 2016
@nathan-at-least nathan-at-least modified the milestones: Temporary overflow, Consensus Code Security Freeze Jul 14, 2016
@nathan-at-least nathan-at-least modified the milestones: z9 Release - RPC, Audit tasks, Testing improvements, z8 release - core (excl. RPC) audit freeze, functional PoW Aug 6, 2016
@nathan-at-least
Copy link
Contributor

@jl777 thanks for digging into this. We won't be able to make any changes proposed here in the z8 release, although it would be good to have some of these changes in place by z9 or our Beta (z10).

Let's close this ticket and open tickets for more specific tasks:

  • Create a document clarifying the differences at the serialization and p2p-protocol level from Bitcoin to help the auxillary infrastructure ecosystem (tools that interact with bitcoin protocol w/out relying on bitcoind / zcashd).
  • Move ciphertexts to the end of serialization (as per protocol or blockformat changes? #991 (comment))

Did I miss anything else?

TODO: Add tickets for bullet points.

@jl777
Copy link
Author

jl777 commented Aug 6, 2016

ok, i will close this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-consensus Area: Consensus rules A-documentation Area: Documentation protocol spec question
Projects
None yet
Development

No branches or pull requests

6 participants