* Security: stop gossiping failure and attempt times as last_seen times
Previously, Zebra had a single time field for peer addresses, which was
updated every time a peer was attempted, sent a message, or failed.
This is a security issue, because the `last_seen` time should be
"the last time [a peer] connected to that node", so that
"nodes can use the time field to avoid relaying old 'addr' messages".
So Zebra was sending incorrect peer information to other nodes.
As part of this change, we split the `last_seen` time into the
following fields:
- untrusted_last_seen: gossiped from other peers
- last_response: time we got a response from a directly connected peer
- last_attempt: time we attempted to connect to a peer
- last_failure: time a connection with a peer failed
* Implement Arbitrary and strategies for MetaAddrChange
Also replace the MetaAddr Arbitrary impl with a derive.
* Write proptests for MetaAddr and MetaAddrChange
MetaAddr:
- the only times that get included in serialized MetaAddrs are
the untrusted last seen and responded times
MetaAddrChange:
- the untrusted last seen time is never updated
- the services are only updated if there has been a handshake
* stop panicking on invalid orchard nullifiers
* add context to error
* use `from_bytes_wide` for nullifiers in arbitrary
* orchard::Nullifier vec to array conversion is a bit clearer and simpler
Co-authored-by: teor <teor@riseup.net>
* Add a `Transaction::version` getter
Returns the version of the transaction as a `u32`.
* Add `Transaction::is_overwintered` helper method
Returns if the `fOverwintered` flag should be set for the transaction's
version.
* Use new helpers to serialize transaction version
Reduce the repeated code and make it less error-prone with future
changes.
* Add getter methods to `transaction::Request` type
Refactor to move the type deconstruction code into the `Request` type.
The main objective is to make it easier to split the call handler into
methods that receive the request directly.
* Refactor to create `verify_v4_transaction` helper
Split the code specific to V4 transactions into a separate helper
method.
* Create `verify_v5_transaction` helper method
Prepare a separate method to have the validation code.
* Add `UnsupportedByNetworkUpgrade` error variant
An error for when a transaction's version isn't supported by the network
upgrade of the block it's included or for the current network upgrade if
the transaction is for the mempool.
* Verify a V5 transaction's network upgrade
For now, only NU5 supports V5 transactions.
* Test that V5 transaction is rejected on Canopy
Create a fake V5 transaction and try to verify it using a block height
from Canopy's activation. The verifier should reject the transaction
with an error saying that the network upgrade does not support that
transaction version.
* Test if V5 tx. is accepted after NU5 activation
Create a fake V5 transaction and pretend it is placed in a block that
has a height after the NU5 activation. The test should succeed, but
since the NU5 activation height has not been specified yet (neither for
the testnet nor the mainnet), for now this test is marked as
`should_panic`.
* Add `TODO` comment to the code
Add more detail to what's left to do, and link to the appropriate PRs.
* Use `u32` to store transaction version
Use a type consistent with how the version is specified.
Co-authored-by: teor <teor@riseup.net>
Co-authored-by: teor <teor@riseup.net>
* Only advance the outbound connection timer when it returns an address
Previously, we were advancing the timer even when we returned `None`.
This created large wait times when there were no eligible peers.
* Refactor to avoid overlapping sleep timers
* Add a maximum next peer delay test
Also refactor peer numbers into constants.
* Make the number of proptests overridable by the standard env var
Also cleanup the test constants.
* Test that skipping peer connections also skips their rate limits
* Allow an extra second after each sleep on loaded machines
macOS VMs seem to need this extra time to pass their tests.
* Restart test time bounds from the current time
This change avoids test failures due to cumulative errors.
Also use a single call to `Instant::now` for each test round.
And print the times when the tests fail.
* Stop generating invalid outbound peers in proptests
The candidate set proptests will fail if enough generated peers are
invalid for outbound connections.
* add zcash_history.rs with librustzcash Tree wrapper
* Apply suggestions from code review
Co-authored-by: teor <teor@riseup.net>
* Apply changes from code review
* Update zebra-chain/src/primitives/zcash_history.rs
Co-authored-by: teor <teor@riseup.net>
* Apply changes from code review
* Add Entry struct; return Result where needed; add test
* Apply suggestions from code review
Co-authored-by: teor <teor@riseup.net>
* zcash_history: improve naming style with `inner`
* zcash_history: check if block has the correct network upgrade when adding to tree
* zcash_history: test improvements
* zcash_history: split Tree::new into new_from_block and new_from_cache
* zcash_history: move tests to their own file
* remove unneeded empty line in Cargo.toml
Co-authored-by: teor <teor@riseup.net>
- Add a custom semver match for `zebrad` versions
- Prefer "line contains string" matches, so tests ignore minor changes
- Escape regex meta-characters when a literal string match is intended
- Rename test functions so they are more precise
- Rewrite match internals to remove duplicate code and enable custom matches
- Document match functions
Rust atomics have an API that's very easy to use incorrectly, leading to
hard to find bugs. For that reason, it's best to avoid it unless there's
a good reason not to.
* implement and test a rate limit in `request_genesis()`
* add `request_genesis_is_rate_limited` test to sync
* add ensure_timeouts constraint for GENESIS_TIMEOUT_RETRY
* Suppress expected warning logs in zebrad tests
Co-authored-by: teor <teor@riseup.net>
* Rename field to `wait_next_handshake`
Make the name a bit more clear regarding to the field's purpose.
* Move `MIN_PEER_CONNECTION_INTERVAL` to `constants`
Move it to the `constants` module so that it is placed closer to other
constants for consistency and to make it easier to see any relationships
when changing them.
* Rate limit calls to `CandidateSet::update()`
This effectively rate limits requests asking for more peer addresses
sent to the same peer. A new `min_next_crawl` field was added to
`CandidateSet`, and `update` only sends requests for more peer addresses
if the call happens after the instant specified by that field. After
sending the requests, the field value is updated so that there is a
`MIN_PEER_GET_ADDR_INTERVAL` wait time until the next `update` call
sends requests again.
* Include `update_initial` in rate limiting
Move the rate limiting code from `update` to `update_timeout`, so that
both `update` and `update_initial` get rate limited.
* Test `CandidateSet::update` rate limiting
Create a `CandidateSet` that uses a mocked `PeerService`. The mocked
service always returns an empty list of peers, but it also checks that
the requests only happen after expected instants, determined by the
fanout amount and the rate limiting interval.
* Refactor to create a `mock_peer_service` helper
Move the code from the test to a utility function so that another test
will be able to use it as well.
* Check number of times service was called
Use an `AtomicUsize` shared between the service and the test body that
the service increments on every call. The test can then verify if the
service was called the number of times it expected.
* Test calling `update` after `update_initial`
The call to `update` should be skipped because the call to
`update_initial` should also be considered in the rate limiting.
* Mention that call to `update` may be skipped
Make it clearer that in this case the rate limiting causes calls to be
skipped, and not that there's an internal sleep that happens.
Also remove "to the same peers", because it's more general than that.
Co-authored-by: teor <teor@riseup.net>
* RFC: add more background to atomics
- Provide explicit tested alternatives to atomics
- Explain the differences between x86 and other processors
* Add nomicon link and further explanation
* Rate-limit new outbound peer connections
Set the rate-limiting sleep timer to use a delay added to the maximum
between the next peer connection instant and now. This ensures that the
timer always sleeps at least the time used for the delay.
This change fixes rate-limiting new outbound peer connections, since
before there could be a burst of attempts until the deadline progressed
to the current instant.
Fixes#2216
* Create `MetaAddr::alternate_node_strategy` helper
Creates arbitrary `MetaAddr`s as if they were network nodes that sent
their listening address.
* Test outbound peer connection rate limiting
Tests if connections are rate limited to 10 per second, and also tests
that sleeping before continuing with the attempts still respets the rate
limit and does not result in a burst of reconnection attempts.
* Add a `at_least_one!` macro for testing
Similar to the `vec!` macro, but doesn't allow creating an empty list.
* Test if `has_inputs_and_outputs` considers actions
Create a dummy transaction with no inputs and no outputs, and add a
dummy Orchard action to it. The `check::has_inputs_and_outputs`
should succeed, because the consensus rule considers having Orchard
actions as having inputs and/or outputs.
* Refactor to create helper function
Move the code to create a fake Orchard shielded data instance to a
helper function in `zebra_chain::transaction::arbitrary`, so that other
tests can also use it.
* Test coinbase V5 transaction with enable spends
A V5 coinbase transaction that has Orchard shielded data MUST NOT have
the enable spends flag set.
* Test if coinbase without enable spends is valid
A coinbase transaction with Orchard shielded data and without the enable
spends flag set should be valid.
* Add a security comment about the `at_least_one!` macro
This macro must not be used outside tests, because it allows memory denial
of service.
Co-authored-by: teor <teor@riseup.net>
* Add sapling final root test vectors
Also tidy some formatting and imports
* Doc: final sapling roots can be duplicated
* Reverse the byte order of final sapling root test vectors
This makes the test vectors match the byte order in the block header,
rather than the zcashd RPC responses.
* Ignore pre-sapling block header commitments
Previously, Zebra expected this reserved field to be all zeroes,
but some mainnet and testnet blocks had other values.
* Test structural and semantic validation of the block commitment field
History roots are excluded from these tests, because they require
contextual validation.
Updates:
- GitHub Issue templates
- GitHub PR templates
- RFC template
Focusing on:
- consensus rule / network reference sections
- design sections
- review/test checklist
Process changes:
- add new team members to RFC approval
- change RFC approval to "most of the team"
And general cleanup:
- delete docs from the checklist, because we now `warn(missing_docs)`
- shorter explanations
- consistent headings
- consistent order
- consistent formatting
* Implement `PartialEq<i64>` for `Amount`
Allows to compare an `Amount` instance directly to an integer.
* Add `SerializationError::BadTransactionBalance`
Error variant representing deserialization of a transaction that doesn't
conform to the Sapling consensus rule where the balance MUST be zero if
there aren't any shielded spends and outputs.
* Validate consensus rule when deserializing
Return an error if the deserialized V4 transaction has a non-zero value
balance but doesn't have any Sapling shielded spends nor outputs.
* Add consensus rule link to field documentation
Describe how the consensus rule is validated structurally by
`ShieldedData`.
* Clarify that `value_balance` is zero
Make the description more concise and objective.
Co-authored-by: Alfredo Garcia <oxarbitrage@gmail.com>
* Update field documentation
Include information about how the consensus rule is guaranteed during
serialization.
Co-authored-by: teor <teor@riseup.net>
* Remove `check::sapling_balances_match` function
The check is redundant because the respective consensus rule is
validated structurally by `ShieldedData`.
* Test deserialization of invalid V4 transaction
A transaction with no Sapling shielded spends and no outputs but with a
non-zero balance value should fail to deserialize.
* Change least-significant byte of the value balance
State how the byte index is calculated, and change the least
significant-byte to be non-zero.
Co-authored-by: teor <teor@riseup.net>
* Standardise lints across Zebra crates, and add missing docs
The only remaining module with missing docs is `zebra_test::command`
* Todo -> TODO
* Clarify what a transcript ErrorChecker does
Also change `Error` -> `BoxError`
* TransError -> ExpectedTranscriptError
* Output Descriptions -> Output descriptions
* Add a `Transaction::orchard_shielded_data` getter
Allows accessing the Orchard shielded data if it is present in the
transaction, regardless of the transaction version.
* Refactor `orchard_nullifiers` to use new getter
Allows making the method more concise.
* Add `CoinbaseHasEnableSpendsOrchard` error variant
Used when the validation rule is not met.
* Implement `enableSpendsOrchard` in coinbase check
The flag must not be set for the coinbase transaction.
* Refactor `Transaction::orchard_*` getters
Use the fact that `Option<T>` implements `Iterator<T>` to simplify the
code and remove the need for boxing the iterators.
Co-authored-by: teor <teor@riseup.net>
* Make sure the Canopy activation block is a finalized checkpoint block
This enables ZIP-221 chain history from Canopy activation onwards.
* Clarify that the mandatory checkpoint test includes Canopy activation
The test was correct, but the docs and assertion message did not include activation.
* Document that the mandatory checkpoint includes Canopy activation
Co-authored-by: teor <teor@riseup.net>
* Restore SummaryDebug on arbitrary chains
And also add it to some more proptest vectors.
* Reduce most arbitrary vectors from 10 to 4
This makes debugging easier
* Make SummaryDebug generic over collections and exact size iterators
* Document DisplayToDebug
* Fix documentation comment
Was missing a slash to become documentation.
* Add documentation link to type reference
Just to help navigation a bit.
* Implement `Transaction::orchard_actions()` getter
Returns an iterator to iterator over the actions in the Orchard shielded
data (if there is one, otherwise it returns an empty iterator).
* Add V5 support for `has_inputs_and_outputs`
Checks if the transaction has Orchard actions. If it does, it is
considered to have inputs and outputs.
* Refactor transaction test vectors
Make it easier to reuse the fake V5 transaction converter in other test
vectors.
* Move helper function to `zebra-chain` crate
Place it together with some other helper functions, including the one
that actually creates the fake V5 transaction.
* Test transaction with no inputs
`check::has_inputs_and_outputs` should return an error indicating that
the transaction has no inputs.
* Test transaction with no outputs
`check::has_inputs_and_outputs` should return an error indicating that
the transaction has no outputs.
* Note that transaction is fake in `expect` message
Should make the message easier to find, and also gives emphasis to the
fact that the transaction is a fake conversion to V5.
Co-authored-by: teor <teor@riseup.net>
Co-authored-by: teor <teor@riseup.net>
* add nullifier methods to orchard
* store orchard nullifiers
* bump database version
* update `IntoDisk`
* support V5 in `UpdateWith`
* add a test for finalized state
* Use the latest network upgrade in state proptests
Given a generated list of gossiped peers, ensure that after running the
`validate_addrs` function none of the resulting peers have a `last_seen`
time that's after the specified limit.
If the calculation to apply the compensation offset overflows or
underflows, the reported times are too distant apart, and could be sent
on purpose by a malicious peer, so all addresses from that peer should
be rejected.
Use some mock gossiped peers where some have `last_seen` times in the
past and some have times in the future. Check that all the peers have
an offset applied to them by the `validate_addrs` function.
This tests if the offset is applied to all peers that a malicious peer
gossiped to us.
teor
Alfredo Garcia
Deirdre Connolly
Janito Vaqueiro Ferreira Filho
dependabot[bot]
Deirdre Connolly
Conrado Gouvea
Pili Guerra