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

Relay and accept mempool transactions based on the ZIP-317 fee rules #5336

Closed
teor2345 opened this issue Oct 4, 2022 · 9 comments · Fixed by #6556
Closed

Relay and accept mempool transactions based on the ZIP-317 fee rules #5336

teor2345 opened this issue Oct 4, 2022 · 9 comments · Fixed by #6556
Assignees
Labels
A-network Area: Network protocol updates or fixes A-rust Area: Updates to Rust code C-security Category: Security issues I-heavy Problems with excessive memory, disk, or CPU usage I-remote-node-overload Zebra can overload other nodes on the network

Comments

@teor2345
Copy link
Contributor

teor2345 commented Oct 4, 2022

Motivation

We want to deploy the new relaying rules in ZIP-317, once enough wallets have updated:

Node developers SHOULD coordinate on schedules for deploying restrictions to their policies for transaction mempool acceptance and peer-to-peer relaying. These policy changes SHOULD NOT be deployed before the changes to transaction inclusion policy by miners described in the preceding paragraph.

https://zips.z.cash/zip-0317#deployment

This should be released in Zebra around the time of the zcashd release that implements ZIP-317, which is release 5.5.0 in April 2023.

We're also adding a minimum fee rate to match zcashd.

Specification

ZIP 317

The relay parts of:

Minimum Fee Rate

zcashd also enforces a standard rule:

  • Transactions must pay a fee of at least 100 zatoshis per 1000 bytes of serialized size, with a maximum fee of 1000 zatoshis
  • In zcashd this is DEFAULT_MIN_RELAY_TX_FEE

In Zebra, we can directly enforce this minimum fee rate during mempool transaction validation.

zcashd details

We don't need to implement exactly what zcashd does.

See Daira's summary here:
#5336 (comment)

Their code is much more complicated:
https://github.com/zcash/zcash/blob/adfc7218435faa1c8985a727f997a795dcffa0c7/src/main.h#L71-L72
https://github.com/zcash/zcash/blob/6ebf01fa831c02c18d6c2cf76c15b0cd2a1fe978/src/main.cpp#L1967-L1971

Design

Zebra currently relays all valid transactions.

We should try to match what zcashd does for their ZIP 317 implementation:

  • transactions below the ZIP-317 fee per action are not relayed or stored in the mempool

The simplest way to do this in Zebra is to add the ZIP-317 check to mempool transaction validation as a standard rule. Then they won't be relayed, because they won't be in the mempool.

Completed

ZIP 401 changes

As part of this ticket, we updated some of the ZIP-401 constants:
zcash/zips#565 (comment)

This was done in

Outdated

Here is the previous behaviour:

Daira says that:

The current threshold used by zcashd, implemented in https://github.com/zcash/zcash/blob/d9aeefc54a3b573eee076e22c44fedeae9e0d6df/src/amount.cpp , is ... a size-based rate
the default value of -minrelaytxfee is 100 zats per KB, not 10. So transactions less than 10000 bytes can be relayed by default with less than a 1000-zat fee

https://discord.com/channels/809218587167293450/809251050741170187/1047520100967780375

@teor2345 teor2345 changed the title Relay mempool transactions based on ZIP-317 Relay mempool transactions based on the ZIP-317 fee rules Oct 4, 2022
@dconnolly
Copy link
Contributor

We want to deploy the new relaying rules in ZIP-317 after most wallets have updated to the new conventional fee.

'The eviction weight is already proportional to the size of the transaction in bytes', so we should be able to deploy these changes before wallets do without negative effect (per daira). What will happen in practice is regardless of whether an attacker chooses big transactions or lots of smaller transactions, the total size of their transactions will be large compared to the legitimate ones, it will always be more likely that the adv. 's transactions will be dropped vs legitimate ones. // @daira

@teor2345 teor2345 added A-rust Area: Updates to Rust code S-blocked Status: Blocked on other tasks P-Medium ⚡ C-security Category: Security issues I-heavy Problems with excessive memory, disk, or CPU usage I-slow Problems with performance or responsiveness I-remote-node-overload Zebra can overload other nodes on the network A-network Area: Network protocol updates or fixes labels Oct 13, 2022
@teor2345
Copy link
Contributor Author

@dconnolly you wrote in the description:

This is not necessarily contingent on a plurality of wallets adopting the new fee:

But the advice in the ZIP is to coordinate deployment of relaying changes with zcashd and wallets.

@teor2345 teor2345 removed the I-slow Problems with performance or responsiveness label Oct 13, 2022
@mpguerra
Copy link
Contributor

@daira
Copy link
Contributor

daira commented Oct 15, 2022

We want to deploy the new relaying rules in ZIP-317 after most wallets have updated to the new conventional fee.

'The eviction weight is already proportional to the size of the transaction in bytes', so we should be able to deploy these changes before wallets do without negative effect (per daira). What will happen in practice is regardless of whether an attacker chooses big transactions or lots of smaller transactions, the total size of their transactions will be large compared to the legitimate ones, it will always be more likely that the adv. 's transactions will be dropped vs legitimate ones. // @daira

This was about mempool eviction (i.e. ZIP 401), not relaying.

@teor2345
Copy link
Contributor Author

This ticket is currently blocked by the deployment of similar changes to wallets.

@teor2345 teor2345 changed the title Relay mempool transactions based on the ZIP-317 fee rules Relay and accept mempool transactions based on the ZIP-317 fee rules Nov 30, 2022
@teor2345
Copy link
Contributor Author

@mpguerra this ticket is ready to go, it would be good to get it in a release soon, so Zebra can stop storing and relaying spam transactions.

It should be very quick, we can just copy the existing check in the mining code to the mempool transaction validator.

@mpguerra
Copy link
Contributor

I will schedule this in for next sprint (2023 Sprint 8) We should make more progress with the audit findings first.

@teor2345 teor2345 removed the S-blocked Status: Blocked on other tasks label Mar 29, 2023
@daira
Copy link
Contributor

daira commented Apr 13, 2023

Minimum Fee Rate
zcashd also enforces a standard rule:

DEFAULT_MIN_RELAY_TX_FEE in zatoshis per kilobyte, in Zebra, this is Amount per 1024 bytes.

For compatibility it should be per 1000 bytes, and at most 1000 zatoshi. In zcash it is currently min(1000, floor(100 * tx_serialized_size / 1000)) zatoshi. That is, for transactions with serialized size 10000 bytes or greater, the relay threshold is 1000 zatoshi (the ZIP 313 standard fee).

In zcashd up to v5.4.2, this relay fee isn't always enforced; there is a complicated system of "free transactions", based on priority calculated using, in the case of fully transparent transactions, the age of the coins being spent. We are currently backporting bitcoin/bitcoin#9602 and related PRs that would simplify this: zcash/zcash#6542 and in particular zcash/zcash@d8a2154 .

@teor2345
Copy link
Contributor Author

Minimum Fee Rate
zcashd also enforces a standard rule:
DEFAULT_MIN_RELAY_TX_FEE in zatoshis per kilobyte, in Zebra, this is Amount per 1024 bytes.

For compatibility it should be per 1000 bytes, and at most 1000 zatoshi. In zcash it is currently min(1000, floor(100 * tx_serialized_size / 1000)) zatoshi. That is, for transactions with serialized size 10000 bytes or greater, the relay threshold is 1000 zatoshi (the ZIP 313 standard fee).

Thanks, I think @oxarbitrage will appreciate these details.

I don't think we want any free transactions in Zebra, we can just apply the relay fee to all mempool transactions.

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-security Category: Security issues I-heavy Problems with excessive memory, disk, or CPU usage I-remote-node-overload Zebra can overload other nodes on the network
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants