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

Stop converting Message::Inv(TxId+) into Request::TransactionsById #2660

Merged
merged 2 commits into from
Aug 24, 2021

Conversation

teor2345
Copy link
Contributor

Motivation

Message::Inv(TxId+) is a transaction advertisement, so it should be converted into Request::AdvertiseTransactionIds.

This is a copy-paste mistake from the original zebra-network implementation.

This bug is unexpected work in sprint 17.

Specifications

The “inv” message (inventory message) transmits one or more inventories of objects known to the transmitting peer. It can be sent unsolicited to announce new transactions or blocks...

https://developer.bitcoin.org/reference/p2p_networking.html#inv

Designs

/// Advertise a set of unmined transactions to all peers.
///
/// This is intended to be used in Zebra with a single transaction at a time
/// (set of size 1), but multiple transactions are permitted because this is
/// how we interpret advertisements from zcashd, which sometimes advertises
/// multiple transactions at once.
///
/// This is implemented by sending an `inv` message containing the
/// unmined transaction ID, allowing the remote peer to choose whether to download
/// it. Remote peers who choose to download the transaction will generate a
/// [`Request::TransactionsById`] against the "inbound" service passed to
/// [`zebra_network::init`].
///
/// v4 transactions use a legacy transaction ID, and
/// v5 transactions use a witnessed transaction ID.
///
/// The peer set routes this request specially, sending it to *every*
/// available peer.
///
/// # Returns
///
/// Returns [`Response::Nil`](super::Response::Nil).
AdvertiseTransactionIds(HashSet<UnminedTxId>),

Solution

  • use AdvertiseTransactionIds for inbound Inv requests from peers

Review

@jvff can review this PR.
@oxarbitrage might also be interested, because this fix is needed for #1077

Reviewer Checklist

  • Code implements Specs and Designs

Follow Up Work

We'll use and test this message in #1077

`Message::Inv(TxId+)` is a transaction advertisement,
so it should be converted into `Request::AdvertiseTransactionIds`.

This is a copy-paste mistake from the original zebra-network
implementation.
@teor2345 teor2345 added C-bug Category: This is a bug A-rust Area: Updates to Rust code P-Medium I-invalid-data Zebra relies on invalid or untrusted data, or sends invalid data A-network Area: Network protocol updates or fixes labels Aug 23, 2021
@teor2345 teor2345 added this to the 2021 Sprint 17 milestone Aug 23, 2021
@teor2345 teor2345 requested a review from jvff August 23, 2021 22:29
@teor2345 teor2345 self-assigned this Aug 23, 2021
@teor2345 teor2345 enabled auto-merge (squash) August 24, 2021 20:59
@teor2345 teor2345 merged commit 0475762 into main Aug 24, 2021
@teor2345 teor2345 deleted the advertise-tx-ids branch August 24, 2021 21:40
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.

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-network Area: Network protocol updates or fixes A-rust Area: Updates to Rust code C-bug Category: This is a bug I-invalid-data Zebra relies on invalid or untrusted data, or sends invalid data
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants