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

Switch to a (txid, auth_digest) identifier in places where the node needs to commit to a specific instance of a transaction #5198

Open
1 task
str4d opened this issue May 28, 2021 · 2 comments
Labels
I-dos Problems and improvements with respect to Denial-of-Service. NU5 Network upgrade: NU5-specific tasks

Comments

@str4d
Copy link
Contributor

str4d commented May 28, 2021

ZIP 244 changes how txids are derived (for v5 txs onward), to enable committing to the effects of a transaction separately from its specific contents (as auth data can be malleated, e.g. re-signing a transaction). The places where zcashd assumes a txid has this effect-committing property, don't need to be changed.

However, zcashd also assumes a txid has an instance-committing property in other places, such as for mempool DoS prevention, as well as potentially other places inherited from bitcoind. If these use txids for v5 transactions, there is a user DoS problem:

  • User submits a valid v5 transaction to the network.
  • An adversary manipulates the authorizing data for the in-flight transaction, making it invalid.
  • zcashd nodes reject the transaction as invalid, and add its txid to the recently-rejected list.
  • The user cannot resubmit their transaction because its txid is blocked.

Upstream had a similar problem for SegWit transactions, which they addressed four years after deploying SegWit by adding P2P support for relaying transactions via wtxid (BIP 339, impl). This works because wtxid is a commitment to a Bitcoin SegWit transaction instance, including effecting data.

In our case, we opted in ZIP 244 to instead have an auth_digest that commits to just the authorizing data of a transaction. The tuple (txid, auth_digest) is our equivalent of Bitcoin's wtxid. We should use this tuple wherever we need to reference a transaction instance.


For the P2P changes, see #5199. This issue is for identifying and fixing code internal to zcashd that needs to be updated:

  • CTxMempool::IsRecentlyEvicted
@str4d str4d added I-dos Problems and improvements with respect to Denial-of-Service. NU5 Network upgrade: NU5-specific tasks labels May 28, 2021
@daira
Copy link
Contributor

daira commented May 29, 2021

For mempool DoS limitation, isn't what we're evicting an equivalence set of possible transactions with the same effect? For v5 transactions, the mempool operates in terms of such equivalence sets anyway. So I think evicting by txid is actually correct. Remember that transactions have to be valid in order to ever be evicted. The fact that a user can't resubmit an evicted transaction is expected behaviour, regardless of whether the witness data was changed.

That is independent of the problem with invalid transactions. We definitely shouldn't consider a txid to be invalid just because we have seen a transaction with that txid that has invalid witness data.

(We MAY cache that a txid is invalid if the invalidity was independent of witness data. At first glance I think that's unlikely to be useful, since witness-independent validity checks don't involve any expensive cryptographic operations AFAIK.)

@str4d str4d added this to the Core Sprint 2021-30 milestone Jul 28, 2021
@str4d str4d added the S-committed Status: Planned work in a sprint label Aug 30, 2021
@r3ld3v r3ld3v added S-committed Status: Planned work in a sprint and removed S-committed Status: Planned work in a sprint labels Oct 13, 2021
@str4d str4d removed this from the Core Sprint 2021-44 milestone Nov 22, 2021
@str4d str4d removed the S-committed Status: Planned work in a sprint label Nov 22, 2021
@daira
Copy link
Contributor

daira commented Aug 8, 2022

@str4d, can this be closed? That is, are we confident that all cases where we should be using wtxids have been found?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I-dos Problems and improvements with respect to Denial-of-Service. NU5 Network upgrade: NU5-specific tasks
Projects
None yet
Development

No branches or pull requests

3 participants