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.

Sign up for GitHub

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

Jump to bottom

Represent coinbase inputs explicitly and download blocks. #240

Merged
merged 11 commits into from
Feb 14, 2020
Merged

Represent coinbase inputs explicitly and download blocks. #240

merged 11 commits into from
Feb 14, 2020

Conversation

hdevalence
Copy link
Contributor

@hdevalence hdevalence commented Feb 8, 2020 •
edited
Loading

Represent and parse coinbase inputs explicitly, then follow on with some code that tests this on the live network:

  • Adds a FindBlocks request / BlockHeaderHashes response;
  • Adds code to the connect stub that blindly downloads blocks;
  • Streamlines the peer connection logic to connect to multiple peers concurrently when trying to increase the size of the peer set.

Closes #241 by incorporation.

@hdevalence hdevalence force-pushed the coinbase branch 2 times, most recently from 86ba2bf to d1006a6 Compare February 8, 2020 21:32
@hdevalence hdevalence changed the base branch from extract-handler to main February 10, 2020 17:07
@hdevalence hdevalence changed the title Represent coinbase inputs explicitly. Represent coinbase inputs explicitly and download blocks. Feb 10, 2020
@hdevalence hdevalence marked this pull request as ready for review February 10, 2020 17:24
@hdevalence
Copy link
Contributor Author

Closes #236.

This was referenced Feb 10, 2020
Closed
Closed
@hdevalence
Represent coinbase inputs explicitly.
ae65643 
Coinbase inputs are handled differently from other inputs and have
different consensus rules, so they should be represented differently in
the source code.  This lets us discard extraneous details (for instance,
it's not necessary to maintain the all-zero hash) and specialize logic.
@hdevalence
Add test vectors for genesis & early blocks.
56edc1d
@hdevalence
Parse block heights in coinbase transactions.
3857d83 
BIP34, which is included in Zcash, encodes the block height into each
block by adding it into the unused BitcoinScript field of the block's
coinbase transaction.  However, this is done just by requiring that the
script pushes the block height onto the stack when it executes, and
there are multiple different ways to push data onto the stack in
BitcoinScript.  Also, the genesis block does not include the block
height, by accident.

Because we want to *parse* transactions into an algebraic data type that
encodes their structural properties, rather than allow possibly-invalid
data to float through the internals of our node, we want to extract the
block height upfront and store it separately from the rest of the
coinbase data, which is inert.  So the serialization code now contains
just enough logic to parse BitcoinScript-encoded block heights, and
special-case the encoding of the genesis block.

Elsewhere in the source code, the `LockTime` struct requires that we
must use block heights less than 500,000,000 (above which the number is
interpreted as a unix timestamp, not a height).  To unify invariants, we
ensure that the parsing logic works with block heights up to
500,000,000, even though these are unlikely to ever be used for Zcash.
@hdevalence
Remove version fields from GetBlocks, GetHeaders.
85c6787 
These are instead set by the negotiated version.
@hdevalence
Add a FindBlocks request to get initial block hashes.
126ec2e 
Bitcoin does this either with `getblocks` (returns up to 500 following block
hashes) or `getheaders` (returns up to 2000 following block headers, not
just hashes).  However, Bitcoin headers are much smaller than Zcash
headers, which contain a giant Equihash solution block, and many Zcash
blocks don't have many transactions in them, so the block header is
often similarly sized to the block itself.  Because we're
aiming to have a highly parallel network layer, it seems better to use
`getblocks` to implement `FindBlocks` (which is necessarily sequential)
and parallelize the processing of the block downloads.
@hdevalence
Add convenience methods to Transaction.
8b711ed 
Because we represent each transaction version as a different variant of the
Transaction enum, we end up in a situation where fields that are common to
different transaction versions are awkward to access, requiring a match
statement with identical match arms.

To fix this, this commit adds the following convenience methods:

* `Transaction::inputs() -> impl Iterator<Item=&TransparentInput>`;
* `Transaction::outputs() -> impl Iterator<Item=&TransparentOutput>`;
* `Transaction::lock_time() -> LockTime`;
* `Transaction::expiry_height() -> Option<ExpiryHeight>`;

The last returns an `Option` because the field is only present in V3 and V4
transactions.

There are some remaining fields that do not get common accessors, because it
probably doesn't make sense to access independently of knowing the transaction
version: `joinsplit_data`, `shielded_data`, `value_balance`.
@hdevalence
Add a Block::coinbase_height() method.
43e9418
@hdevalence
Add PartialOrd, Ord to BlockHeight.
00016ab
@hdevalence
Exercise network functionality by downloading lots of blocks.
695cec3 
(Don't check any information about them, just blindly download).
@hdevalence
Connect to multiple peers concurrently.
8a5fcae 
The previous outbound peer connection logic got requests to connect to new
peers and processed them one at a time, making single connection attempts
and retrying if the connection attempt failed.  This was quite slow, because
many connections fail, and we have to wait for timeouts.  Instead, this logic
connects to new peers concurrently (up to 50 at a time).
@hdevalence
Copy link
Contributor Author

Rebased onto main to pick up #246.

@dconnolly dconnolly self-assigned this Feb 13, 2020
@dconnolly dconnolly self-requested a review February 13, 2020 20:43
dconnolly
dconnolly approved these changes Feb 14, 2020
View reviewed changes
Copy link
Contributor

@dconnolly dconnolly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚢

zebra-chain/src/transaction/serialize.rs
@@ -15,6 +15,13 @@ use super::*;
const OVERWINTER_VERSION_GROUP_ID: u32 = 0x03C4_8270;
const SAPLING_VERSION_GROUP_ID: u32 = 0x892F_2085;

const GENESIS_COINBASE_DATA: [u8; 77] = [
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😭

zebra-chain/src/transaction/serialize.rs
BlockHeight(data[1] as u32 + ((data[2] as u32) << 8) + ((data[3] as u32) << 16)),
data.split_off(4),
)),
// The genesis block does not encode the block height by mistake; special case it.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

zebra-chain/src/transaction/serialize.rs
}

fn coinbase_height_len(height: BlockHeight) -> usize {
// We can't write this as a match statement on stable until exclusive range
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

zebra-chain/src/transaction/serialize.rs
self.previous_output.zcash_serialize(&mut writer)?;
self.signature_script.zcash_serialize(&mut writer)?;
writer.write_u32::<LittleEndian>(self.sequence)?;
match self {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

zebra-chain/src/transaction/serialize.rs
signature_script: Script::zcash_deserialize(&mut reader)?,
sequence: reader.read_u32::<LittleEndian>()?,
})
// This inlines the OutPoint deserialization to peek at the hash value
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

zebra-network/src/peer/connection.rs
@@ -81,6 +82,15 @@ impl Handler {
Finished(Err(Arc::new(PeerError::WrongBlock).into()))
}
}
(FindBlocks, Message::Inv(inv_hashes)) => Finished(Ok(Response::BlockHeaderHashes(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

zebra-network/src/peer/connection.rs
@@ -317,6 +327,15 @@ where
tx,
)
}),
(AwaitingRequest, FindBlocks { known_blocks, stop }) => self
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

zebra-network/src/peer_set/initialize.rs
// <FuturesUnordered as Stream> returns None when empty.
// Keeping an unresolved future in the pool means the stream
// never terminates.
handshakes.push(future::pending().boxed());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

zebra-network/src/peer_set/initialize.rs
}
}
// did a drill sergeant write this? no there's just no Either3
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤣

zebrad/src/lib.rs
@@ -6,6 +6,9 @@

//#![deny(warnings, missing_docs, trivial_casts, unused_qualifications)]
#![forbid(unsafe_code)]
// Tracing causes false positives on this lint:
// https://github.com/tokio-rs/tracing/issues/553
#![allow(clippy::cognitive_complexity)]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@dconnolly dconnolly merged commit b443d7a into main Feb 14, 2020
@dconnolly dconnolly deleted the coinbase branch February 14, 2020 23:23
@hdevalence hdevalence mentioned this pull request Feb 19, 2020
Merged
skyl added a commit to skyl/zebra that referenced this pull request Sep 25, 2024
@skyl
Refactor all modals / Dialogs (ZcashFoundation#240)
a59b5e5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dconnolly dconnolly dconnolly approved these changes

Assignees

@dconnolly dconnolly

Labels
None yet
Projects
None yet
Milestone
Block Validation
Development

Successfully merging this pull request may close these issues.
2 participants
@hdevalence @dconnolly