Enable packages through testmempoolaccept (rpc/rest/zmq, tests, validation)

https://github.com/bitcoin/bitcoin/pulls/20833

Host: glozow  -  PR author: glozow

The PR branch HEAD was ebca80d at the time of this review club meeting.

Definitions

  • A package is a group of transactions that have some dependency relationship, i.e., some transaction spends the outputs of another transaction in the package.

  • A test accept of validation refers to an invocation of mempool validation that validates but does not submit a transaction to the mempool, called from the testmempoolaccept RPC.

  • A Coin (CCoin) is a UTXO. In our context, coins don’t necessarily need to exist on-chain; they can be outputs from transactions in the mempool or some other valid but unconfirmed transaction. A Coins View is a data structure that contains the available coins. The typical usage is to query a Coins View with a COutPoint.

Background

  • The testmempoolaccept RPC was introduced in #11742 and allows users to test raw transactions for validity. While the return object is an array (with the intention to allow multiple transactions), testmempoolaccept is currently limited to one transaction at a time, which means users cannot test transactions that depend on outputs that the mempool has not seen yet. The main reason for this limitation is that our validation code only uses the coins available in the current chain state and mempool (and not anything else), to validate transaction inputs.

  • #20833 is also intended to be a first step towards Package Mempool Accept and hopefully, eventually, Package Relay. We have discussed Package Relay in a previous review club, #16401. You can read more about Package Mempool Accept here.

Notes

  • #20833 enables dry runs of package validation. It does this by having CCoinsViewMemPool keep track of the Coins created and spent by transactions in the package.

  • The MemPoolAccept class represents a “validation session” with the current mempool and chain state; it holds cs_main and pool.cs mutexes throughout validation. Its job is to figure out whether a transaction (received from peers or clients like RPC) is a valid candidate for submission to the mempool. To do so, it applies consensus and policy rules. Importantly, if a transaction is invalid, MemPoolAccept is designed such that it fails as quickly as possible to mitigate DoS risks.

  • Each MemPoolAccept instance creates a Coins View from the current chain tip and mempool. CCoinsView is a class with a hierarchical structure (see diagram below).

             +--------------------+
             |                    |
             |     CCoinsView     |
             |                    |
             +----------+---------+
                        ^
                        |
             +----------+---------+
             |                    |
             |  CCoinsViewBacked  |
             |                    |
             +------+------+------+
                    ^      ^
           +--------+      +--------+
           |                        |
+----------+---------+  +-----------+---------+
|                    |  |                     |
|  CCoinsViewCache   |  |  CCoinsViewMemPool  |
|                    |  |                     |
+--------------------+  +---------------------+
  • A CCoinsViewBacked points to another CCoinsView as its “backend.” When CCoinsViewBacked.GetCoin() cannot find a coin in its own data structures, it calls backend.GetCoin() to continue searching. Thanks to the magic of polymorphism, CCoinsViewBacked Coins Views can be “layered” to allow searching for coins in multiple locations without changing the underlying chain state. We have discussed the layered UTXO cache in a previous review club, #17487.

  • Since the UTXO set is quite large and stored on disk, Bitcoin Core uses a cache to improve validation performance. To prevent cache thrashing caused by an attacker broadcasting many invalid transactions spending a wide variety of UTXOs (a cheap DoS attack), we keep track of which Coins are brought in from disk and uncache them if the transaction turns out to be invalid.

  • Validation roughly consists of PreChecks, PolicyScriptChecks, ConsensusScriptChecks, and Finalize. We’ve touched on these functions in previous review clubs, e.g. #19339. This Stack Exchange post about consensus vs policy rules may also be helpful.

  • In an effort to not make any API-breaking changes, #20833 returns a results array with one result per transaction in the same order they were passed in or the first failure result.

Questions

  1. Did you review the PR? Concept ACK, approach ACK, tested ACK, or NACK?

  2. What are some use cases for enabling packages through testmempoolaccept?

  3. What are some limitations of this implementation of package validation?

  4. From a code quality standpoint, what is the benefit of moving the validation state, replaced transactions, and fee to Workspace? Should we add coins_to_uncache to Workspace and make ATMPArgs a const? Why or why not?

  5. Why do we run all PreChecks before script checks? Would it still be correct if we didn’t do this?

  6. In AcceptSingleTransaction (with test_accept=true), both PolicyScriptChecks and ConsensusScriptChecks are run. However, in AcceptMultipleTransactions, we don’t run ConsensusScriptChecks. Why is that okay? (Followup question: why do we run ConsensusScriptChecks at all in ATMP?)

  7. When we fetch a coin from m_viewmempool, what 4 places do we look and in what order? Why do we check cache_package_remove before cache_package_add in CCoinsViewMemPool::GetCoin?

  8. If a block arrives while we’re running PreChecks, do we have to worry about our CoinsView becoming inconsistent?

  9. Why do we set our CoinsView backend to dummy (code)? What bug is this trying to prevent?

  10. What are your thoughts on the results array (returning one for each tx if successful or the first one if failed)? Do you have any suggestions for improvement, and is it API-breaking?

Meeting Log

  117:00 <glozow> #startmeeting
  217:00 <emzy> hi
  317:00 <glozow> Welcome to PR Review Club everyone!
  417:00 <jnewbery> hi!
  517:00 <Murch> Hello
  617:00 <murtyjones> hiya
  717:00 <norisgOG> hi!
  817:00 <robot-dreams> hi!
  917:00 <AnthonyRonning> hello
 1017:00 <svav> Hi
 1117:00 <lightlike> hi
 1217:00 <glozow> Feel free to say hi so we know you're here :) Any first-timers?
 1317:00 <sepoyi> hi
 1417:00 <that> hi
 1517:00 <thomasb06> hi
 1617:00 <ccdle12> hi
 1717:00 <dhruvm> hi
 1817:00 <michaelfolkson> hi
 1917:00 <glozow> Today, we’re looking at #20833: enable 📦 through testmempoolaccept (https://github.com/bitcoin/bitcoin/pull/20833)
 2017:00 <ariard> hi
 2117:00 <sepoyi> first time here
 2217:00 <stacie> hi
 2317:00 <prayank> hi
 2417:00 <glozow> welcome sepoyi!
 2517:00 <wiz> hi
 2617:01 <glozow> Did anyone get a chance to review the PR? y/n
 2717:01 <AnthonyRonning> y
 2817:01 <emzy> n
 2917:01 <robot-dreams> y
 3017:01 <murtyjones> notes yes, PR no
 3117:01 <norisgOG> n
 3217:01 <stacie> y
 3317:01 <thomasb06> y
 3417:01 <svav> y
 3517:01 <sepoyi> y
 3617:01 <glozow> btw this club is dedicated to learning, so you’re always welcome to ask any questions at any time :)
 3717:01 <jnewbery> welcome sepoyi!
 3817:01 <dhruvm> n
 3917:01 <jnewbery> y
 4017:01 <sishir> hi
 4117:01 <ccdle12> n
 4217:01 <sunon> y
 4317:02 <glozow> ok let's start with some conceptual questions. What are some use cases for enabling packages through testmempoolaccept? (non-package-relay related please, since that’s very far off from this PR)
 4417:02 <sepoyi> thanks glozow jnewbery
 4517:02 <AnthonyRonning> A series of offline transactions that build on each other
 4617:02 <sishir> This allows you to test validity for second-layer application transaction chains (i.e. with multiple spending paths and where you don't want to broadcast yet).it could be helpful to return more information to the client, but don't know what that would look like concretely
 4717:02 <murtyjones> maybe testing channel opening on LN?
 4817:02 <thomasb06> testmempoolaccept will tell whether the transaction would be accepted by the nodes.
 4917:02 <dhruvm> chained transactions for L2 protocols
 5017:03 <glozow> yep! yep!
 5117:03 <robot-dreams> Specifically, double check if the other party cheats in a lightning channel, the subsequent "justice" transaction would get accepted
 5217:03 <robot-dreams> (check the pair of transactions without adding either to the mempool)
 5317:03 <ariard> robot-dreams: not an issue, justice tansactions are fully malleable
 5417:03 <ariard> it's more for counter-signed txn
 5517:04 <stacie> A non-package-relay answer: With packages you can now validate multiple txs with one testmempoolaccept call (which previously accepted an array but but only expected it to be size one)
 5617:04 <ariard> *single-party malleable
 5717:04 <glozow> yeah! we can check transactions that build on each other, although there are some limitations - would anyone like to describe some limitations with what this PR enables?
 5817:04 <sishir> timelocks, off-chain covenants?
 5917:04 <murtyjones> from what i read, parent transactions of the tested one must be in the mempool or chain
 6017:04 <svav> To enable child pays for parent transactions.
 6117:04 <murtyjones> as opposed to being given in the request
 6217:04 <AnthonyRonning> does it requires the transactions in a package to be submitted in order of dependency?
 6317:04 <robot-dreams> ariard: thanks for the clarification
 6417:05 <thomasb06> it only works if the parent transactions are confirmed or in the mempool.
 6517:05 <glozow> yep, sishir named some good ones, ariard has a good comment on this: https://github.com/bitcoin/bitcoin/pull/20833#pullrequestreview-570114670
 6617:05 <lightlike> I think transactions need to be submitted in the right order.
 6717:05 <glozow> and yes, the PR requires you to have them sorted like AnthonyRonning said
 6817:06 <glozow> (and lightlike said)
 6917:06 <stacie> Another limitation is the atomic validation - the response is only for the first failure, so if you have multiple, you'll need to go back and correct each one before calling testmempoolaccept again
 7017:06 <glozow> stacie: correct! this gives you all-or-nothing success or failure
 7117:07 <ariard> stacie: in practice, that's hard to do because it assumes your txn generation backend to fully consume the errors and be able to correct them while still enforcing application semantic
 7217:07 <glozow> alright I think we have a good idea of what this PR does/doesn't do, let's dive into the code
 7317:07 <glozow> The PR starts off with a few refactors. From a code quality standpoint, what is the benefit of moving the validation state, replaced transactions, and fee to `Workspace`? Should we add `coins_to_uncache` to `Workspace `and make `ATMPArgs` a const? Why or why not?
 7417:07 <ariard> beyond fees bumping, I'm not aware of such backend
 7517:08 <AnthonyRonning> For code quality, it’s more explicit than relying on `AcceptToMemoryPool` to fill in memory addresses for passed in state, replaced txs, fee.
 7617:08 <glozow> AnthonyRonning: I like where that's going, yeah! we have the `MemPoolAccept` class fill in the results instead of ATMP
 7717:09 <AnthonyRonning> what does ATMP stand for?
 7817:09 <glozow> what should ATMPArgs be used for, and what should Workspace be used for?
 7917:09 <sunon> AcceptToMemoryPool
 8017:09 <glozow> AnthonyRonning: AcceptToMemoryPool, sorry
 8117:09 <AnthonyRonning> oh yeah duh thanks :)
 8217:09 <stacie> ariard - agree, and I think the atomic validation is a good thing, but I wonder if people will use this new feature to try validating a set of unrelated transactions. The only use case I can see is to save on the network overhead of multiple RPC calls, but it does seem strange to need to batch/package up a bunch of unrelated transactions ¯\_(ツ)_/¯
 8317:11 <AnthonyRonning> I was a bit unsure about the `coins_to_uncache` or const `ATMPArgs` part of the question
 8417:11 <glozow> stacie ariard: yeah, this would still work for unrelated transactions, although that's not what it's intended for. we have a question about API later so we could discuss it further :)
 8517:12 <glozow> cool! So I'll answer the first part, ATMPArgs = arguments for validation, e.g. test_accept. The Workspace gets updated as we go through the steps of validation.
 8617:12 <svav> The Workspace should contain state that may change
 8717:12 <svav> throughout validation.
 8817:12 <glozow> Could someone offer an opinion on which should be const and which should be mutable?
 8917:12 <lightlike> I can see that it makes it makes the code cleaner and easier to read if we separate input arguments and results for ATMP - not sure if that is the only reason for the refactor though?
 9017:13 <ariard> if you modify ATMPArgs at a later steps of the validation process you might invalidate previous ones
 9117:13 <sunon> Args const, workspace mutable?
 9217:14 <glozow> sunon: ding ding! yep
 9317:14 <glozow> ariard: yup yup, it'd be really bad if we're using different validation params at different steps in the process
 9417:15 <glozow> lightlike: yep, the refactor basically makes the code cleaner. no logic change.
 9517:15 <glozow> ok chugging along...
 9617:15 <glozow> Why do we run `PreChecks` for ALL of the transactions before script checks? Would it still be correct if we didn’t do this?
 9717:15 <glozow> Link to the code we're talking about, `AcceptMultipleTransactions`: https://github.com/bitcoin/bitcoin/blob/70f84b22199cd0afffd12ea8e8934d22b0f0c7f4/src/validation.cpp#L1074
 9817:15 <sishir> What’s the difference between policy checks and pre checks?
 9917:15 <sunon> I think it's to fail early without doing unnecessary work
10017:15 <AnthonyRonning> To fail fast and avoid signature checks if one tx in a package fails something basic like input tx or fee.
10117:16 <glozow> sunon: AnthonyRonning: correct!
10217:17 <michaelfolkson> Policy checks are a superset of the pre checks right? The pre checks are policy checks too?
10317:17 <jnewbery> sishir: the PolicyScriptChecks function is validating the scripts and signatures. That's the most computationally expensive part of validating a transaction
10417:17 <glozow> sishir: policy includes non-consensus rules that Bitcoin Core nodes enforce on unconfirmed transactions, mostly to protect from DoS. Some of the checks are in `PreChecks`, and some are in `PolicyScriptChecks`
10517:17 <sishir> michaelfolkson I thought Policy checks are a superset of consensus checks
10617:17 <sunon> Surface for DoS, right?
10717:17 <michaelfolkson> sishir: They are haha
10817:18 <michaelfolkson> But pre checks I think are a subset of policy checks
10917:18 <glozow> yeah you're all correct. Policy checks rules in addition to consensus. PreChecks is a subset of policy checks because it does some of the policy checks haha
11017:18 <michaelfolkson> Phew
11117:18 <michaelfolkson> Quite the brain exercise
11217:18 <glozow> sunon: yes, script checking is really expensive, we try not to do that as much as possible
11317:18 <sishir> i see that makes sense thank you
11417:18 <sunon> Ah thanks I see the venns in my head lol
11517:19 <michaelfolkson> I don't think any pre checks are consensus checks if that helps
11617:19 <glozow> ok, so quick sanity check: would it still be correct if we did PreChecks + PolicyScriptChecks for each transaction?
11717:19 <lightlike> do we need to be concerned about DOS though for a RPC? Or is that in preparation for reusing this code for getting packages from actual peers?
11817:19 <ariard> michaelfolkson: but your policy sets might be disjunctive and your consensus sets extended across versions
11917:19 <jnewbery> well prechecks also rejects transactions for being consensus-invalid
12017:19 <glozow> lightlike: right, we're not too concerned about DoS for an RPC. but this validation code is shared for txns we get through P2P
12117:20 <michaelfolkson> ariard, jnewbery: I stand corrected. Correct those venn diagrams
12217:20 <sunon> It would still be correct to do both
12317:20 <glozow> sunon: yes!
12417:20 <glozow> this is purely to fail faster.
12517:20 <glozow> Next question: In `AcceptSingleTransaction` (with test_accept=true), both `PolicyScriptChecks` and `ConsensusScriptChecks` are run. However, in `AcceptMultipleTransactions`, we don’t run `ConsensusScriptChecks`. Why is that okay?
12617:21 <sishir>  PolicyScriptChecks is stricter so ConsensusScriptChecks is unnecessary.
12717:21 <AnthonyRonning> Since `AcceptMultipleTransactions` does not broadcast the transactions, even with `test_accept=false`, there’s no reason to do consensus checks.
12817:21 <jnewbery> like this for example: https://github.com/bitcoin/bitcoin/blob/15a9df070615e7e8f29c17a92b889f19218f25ac/src/validation.cpp#L793-L807 a transaction that spends the output of transaction it conflicts with can never be valid. That's a consensus rule since such a transaction could never appear in the block chain
12917:21 <glozow> sishir: yes! 🧠
13017:21 <glozow> like you said before, policy is strictly stricter than consensus :)
13117:22 <glozow> Okay but followup question: why do we run `ConsensusScriptChecks` at all in ATMP?
13217:22 <sishir> Follow up:At L2 level we’re aiming for is a txstandardness verifier, a tool encapsulating ATMP checks
13317:23 <michaelfolkson> For those who are confused glozow linked to this StackExchange post on policy and consensus in her notes https://bitcoin.stackexchange.com/questions/100317/what-is-the-difference-between-policy-and-consensus-when-it-comes-to-a-bitcoin-c
13417:23 <glozow> thanks michaelfolkson
13517:23 <glozow> sishir: could you elaborate?
13617:24 <sishir> https://github.com/bitcoin/bitcoin/pull/20833#issuecomment-765057102
13717:24 <AnthonyRonning> thanks michaelfolkson! I was indeed confused on what consensus checked
13817:25 <jnewbery> There are two reasons to run ConsensusScriptChecks. Anyone want to guess what either of those are?
13917:25 <sepoyi> To ensure backward compatibility?
14017:25 <ariard> glozow: I think we don't want to run `ConsensusScriptChecks` because for now those txn aren't added to the mempool and thus there is no matter caching their script results for a potential block validation?
14117:26 <michaelfolkson> Run on your transaction or someone else's transaction jnewbery? Or do you mean in the code?
14217:26 <michaelfolkson> I think you mean in the code
14317:26 <jnewbery> michaelfolkson: I mean in MemPoolAccept
14417:27 <robot-dreams> ariard: That was my first thought too, but then wasn't sure, why would we run it for the `AcceptSingleTransaction` case?
14517:27 <glozow> ariard: yes, it's a test accept so we're not really waiting for it to be in a block. Not much to gain from script caching here
14617:27 <michaelfolkson> You don't want invalid transactions in your mempool. Takes up space, inefficient, could lose you connections if you try to propagate it?
14717:28 <michaelfolkson> Ha I think I'm misunderstanding the question
14817:28 <glozow> Ok. In a for-realsies transaction validation through MemPoolAccept.AcceptSingleTransaction() (e.g. a tx we receive on P2P), we run ConsensusScriptChecks.
14917:29 <glozow> What are the 2 things this accomplishes?
15017:29 <glozow> ariard mentioned one of them
15117:30 <AnthonyRonning> in case a new block comes in?
15217:30 <ariard> have a look in the comment in `ConsensusScriptChecks`
15317:30 <sunon> We want to make sure they're valid txs before adding to our mempool
15417:31 <glozow> ariard's hint at this line: https://github.com/bitcoin/bitcoin/blob/15a9df070615e7e8f29c17a92b889f19218f25ac/src/validation.cpp#L961
15517:31 <glozow> sunon: yes, we want to make sure they're valid. that's 1 reason!
15617:31 <sunon> Caches script results?
15717:31 <glozow> sunon: bingo!!!!!
15817:31 <jnewbery> winner!
15917:31 <glozow> 💃
16017:32 <glozow> even though it's guaranteed to be consensus-valid if it passes all policy checks, we would like to cache the script results!
16117:32 <sunon> Learning things today!
16217:32 <glozow> that means when we see it in a block, we'll have faster validation :)
16317:32 <michaelfolkson> "have a few blocks of extra misses on soft-fork activation" I love a good soft fork reference
16417:32 <glozow> whew! let's move on.
16517:32 <robot-dreams> Sorry, final question about "guaranteed to be consensus-valid", if we can make that guarantee what's the check on line 968 for?
16617:32 <sunon> And no expensive scriptsig checks
16717:33 <glozow> When we fetch a coin from a `CCoinsViewMemPool` (in this PR), what 4 places do we look and in what order? Why do we check `cache_package_remove` before `cache_package_add`?
16817:33 <robot-dreams> s/check/comment* "This is also useful in case of bugs in the standard flags that cause transactions to pass as valid when they're actually invalid."
16917:33 <AnthonyRonning> `cache_package_remove`, `cache_package_add`, mempool, and then backend (disk?). We check removed first because a package tx before could have already spent another package tx’s output.
17017:34 <glozow> robot-dreams: I think that's a "just in case there is a bug even though we don't think there's a bug" type of thing. policy flags as a superset of consensus flags should guarantee this. (btw we had a whole review club on this topic hehe https://bitcoincore.reviews/19698)
17117:34 <glozow> AnthonyRonning: yep! you got all 4!
17217:35 <AnthonyRonning> just a general code question, could there be an a single `cache_package_coin` that both adds and removes as needed? Instead of having 2 lists and iterating through both?
17317:35 <ariard> yet-another belt-and-suspsender which would be worthy to document better
17417:35 <glozow> and yes! we check removed first for precisely that reason, a tx in a package can spend an output created by another tx in the package.
17517:35 <jnewbery> robot-dreams: a consensus-invalid transaction getting into the mempool is bad news for the miners. They'd construct a block including that transaction and then later find out that the block is invalid.
17617:35 <glozow> Well, they'd see that error in TestBlockValidity before finding the nonce yeah?
17717:36 <jnewbery> so this is just an extra check to make sure that doesn't happen. It shouldn't since policy rules should be stricter than consensus rules, but it's good to double check here.
17817:36 <michaelfolkson> I guess double checking before setting your whole ASIC farm on it is nice
17917:37 <glozow> AnthonyRonning: yes, that could work too. Feel free to leave a review comment :) worth exploring
18017:37 <robot-dreams> jnewbery: glozow: Thanks, makes sense that it's a double check
18117:37 <ariard> really-worst case scenario it could be leveraged by an attacker to censor block construction by all other miners for few hours and thus txn
18217:38 <glozow> yeah true, that'd be bad.
18317:38 <glozow> ok next question: If a block arrives while we’re running `PreChecks`, do we have to worry about our CoinsView becoming inconsistent?
18417:38 <AnthonyRonning> No because a mempool read lock is applied before running `PreCheck`
18517:39 <michaelfolkson> Nice
18617:40 <glozow> AnthonyRonning: very close, our mempool shouldn't change. Is there another lock that guards our CoinsView? :)
18717:40 <robot-dreams> cs_main, does this mean you can't add a block until the `Accept` is done?
18817:41 <AnthonyRonning> looks like an assertion on `cs_main` though I'm not sure what guarantees that has
18917:41 <dhruvm> The active chain is also guarded by cs_main
19017:41 <glozow> robot-dreams: AnthonyRonning: dhruvm: correct! cs_main guards all our chain state stuff
19117:41 <AnthonyRonning> ah, cs = chain state!
19217:42 <glozow> and yes, we wouldn't look at the block while we're doing ATMP
19317:42 <robot-dreams> Wait I think cs = critical section
19417:42 <dhruvm> AnthonyRonning: I think cs is for critical section
19517:42 <glozow> yeah cs = critical section
19617:42 <AnthonyRonning> oh cool, thx for correction :)
19717:42 <jnewbery> glozow: on you question about TestBlockValidity, yes it looks like this is called before getblocktemplate returns a block template. Although I don't know what the advice would be to a miner who saw a "TestBlockValidity() failed" error.
19817:43 <glozow> cool, so feel free to grep for `cs_main` in the code everyone :) it's EVERYWHERE
19917:43 <glozow> Next question: Why do we set our CoinsView backend to dummy? What bug is this trying to prevent? (We’re looking at this line: https://github.com/bitcoin/bitcoin/blob/15a9df07/src/validation.cpp#L674)
20017:43 <AnthonyRonning> everything is critical 😂
20117:44 <sishir> if we set backend to dummy, we lose the ability to look up the coins in m_viewmempool
20217:44 <jnewbery> cs is critical section, which is a windows name for a mutex. Satoshi wrote the first implementation for windows
20317:45 <ariard> jnewbery: warn a human to check the logs, you don't know which transaction is faultive, it might even be the header
20417:45 <glozow> sishir: you're right that's why I change it in the PR, but why do we do it in the first place?
20517:45 <michaelfolkson> PR to change the name of cs_main? :)
20617:45 <sunon> We might never uncache coins that we would have pulled from disk
20717:46 <jnewbery> michaelfolkson: no thanks
20817:46 <glozow> sunon: yes! why do we want to uncache coins pulled from disk?
20917:46 <sishir> 🤔
21017:46 <glozow> michaelfolkson: please no
21117:46 <michaelfolkson> jnewbery: Sorry just joking
21217:46 <prayank> AnthonyRonning: Offtopic but Bitcoin Core is ranked as 4th most critical C++ OSS Project
21317:47 <AnthonyRonning> prayank: ahh nice, i am biased but would give it #1 :)
21417:47 <michaelfolkson> Let's stay on topic plz
21517:47 <sishir> is it to protect against bugs?
21617:47 <glozow> prayank: critical referring to important to the world or most amount of critical sections?
21717:47 <ariard> yes bitcoin is to protect against money bug
21817:47 <glozow> yeah let's stay on topic, what bugs sishir?
21917:48 <sunon> We don't want to waste space in utxo cache
22017:48 <glozow> sunon: yeah! can someone tell me what kind of attack someone could launch if we forgot to uncache coins we pulled into the utxo cache?
22117:49 <robot-dreams> Test a package of 974 transactions where the 973rd fails?
22217:49 <murtyjones> some kind of attack exhausting the cache size limit would be my guess
22317:49 <dhruvm> glozow: cache thrashing?
22417:49 <glozow> robot-dreams: murtyjones: dhruvm: yep!
22517:50 <AnthonyRonning> is there any kind of limit to how many txns can be in a package?
22617:50 <glozow> we use our cache for performance, but if the attacker got us to pull a bunch of useless coins into our cache, we'd get very little use out of it
22717:50 <glozow> they could do this without packages, too
22817:50 <dhruvm> murtyjones: i dont think it would exhaust the cache so much as make every cache read be a miss
22917:51 <murtyjones> ah got it
23017:51 <glozow> they could do it for every coin in the UTXO set (since it can be an invalid transaction) and we'd just be getting cache misses all the time
23117:52 <glozow> murtyjones: i suppose it's exhaustive in that we do more disk reads than necessary
23217:52 <glozow> last question before we wrap up for today: What are your thoughts on the results array (returning one for each tx if successful or the first one if failed)? Do you have any suggestions for improvement?
23317:52 <Murch> glozow: Wouldn't a peer that sends us a flood of invalid txs get banned?
23417:52 <lightlike> glozow: would that be an actual attack that would do something worse than slow us down a bit? I'm not so familiar with the topic, but if we have useless full cache, wouldn't we just load more txes from disk until the cache gets better again?
23517:53 <AnthonyRonning> It seems like the API now does what it intended to do, so ACK. Not API-breaking because nobody could do more than 1 before anyways, this expands functionality.
23617:53 <jnewbery> Murch: not necessarily. It depends why the transaction isn't accepted.
23717:53 <robot-dreams> glozow: Another alternative (not necessarily improvement): return an array of the same size, first failure gets input reason, any dependents get `input-missing`?
23817:53 <robot-dreams> any subsequent dependents*
23917:53 <Murch> Oh right, it could be an orphan transaction or smth
24017:54 <jnewbery> We don't ban peers for sending us transactions that are consensus valid but fail our policy checks, or have a fee too low to get into our mempool for example
24117:54 <ariard> yeah I don't get what you don't return package size ?
24217:54 <murtyjones> if there are multiple failures, it could be useful to send all of them back.
24317:54 <Murch> oh right, they could also e.g. just exceed ancestor limits
24417:54 <sishir> im still trying to understand this PR, will we be able to do something like this (node.testmempoolaccept([rawtx1, rawtx2, rawtx3, rawtx4])) once it's merged?
24517:54 <robot-dreams> murtyjones: agreed though similar to how compiler writers want to avoid spamming users with errors if they missed a semicolon (and caused the next 500 lines to all have errors), maybe we want some way to show only "uncorrelated" errors
24617:55 <glozow> lightlike: yeah, it would slow us down, it's not exhausting CPU
24717:55 <jnewbery> murtyjones: we abort at the first failure (no point in trying to validate children transactions if the parent is invalid)
24817:55 <murtyjones> ah okay. forgot that they have to all be related
24917:55 <ariard> Murch: committing to a pairing policy which would ban evict peers for policies divergences would be a violation of the concept itself of policy :)
25017:55 <Murch> murtyjones: that would conflict with the goal to "fail fast"
25117:55 <glozow> sishir: yes, this PR enables that
25217:56 <sishir> i see thats super cool
25317:56 <stacie> Was the original intent of having testmempoolacccept take in an array to support packages? (back when the RPC call was first introduced)
25417:56 <jnewbery> there's not time to get into this in great detail now, but the UTXO cache has a slightly unusual access pattern compared to many caches that people are familiar. The main thing it's saving us is _writes_, not _reads_
25517:56 <Murch> ariard, jnewbery: well, strictly speaking I did write _invalid_ txs :p
25617:56 <glozow> ariard: I think in the future we could have each result obj have that information
25717:57 <michaelfolkson> stacie: I think the use case for testing 1 transaction is sufficient intent
25817:57 <glozow> since they won't necessarily 1 package
25917:57 <glozow> stacie: yes, that was the intent
26017:57 <michaelfolkson> But you could test 1 transaction and that would still be useful right?
26117:57 <ariard> stacie: package is a notion from the mempool, it has been a forever talk to expose it as a p2p thing
26217:57 <glozow> comment from original PR: https://github.com/bitcoin/bitcoin/pull/11742#issuecomment-352842691
26317:57 <jnewbery> if a coin gets spent shortly after it gets created, we never have to write it to disk at all, and many coins get spent in the same block or the next few blocks after they get created (see page 14 of https://eprint.iacr.org/2019/611.pdf)
26417:59 <stacie> ah! I see it in that comment, "A future pull can extend this rpc to take lists of more than one raw transaction and feed them into an ephemeral mempool (e.g. based on the current mempool)." thanks!
26517:59 <dhruvm> jnewbery: interesting. does the low frequency of flushing UTXOs to disk have implications on chainstate corruption in case of unexpected termination?
26618:00 <jnewbery> dhruv: maybe off topic. We can discuss another time!
26718:00 <glozow> if anybody comes up with any other ideas on API, please feel free to comment on the PR!
26818:00 <glozow> that's all we have time for today, thank you all for coming! 🤗
26918:00 <glozow> #endmeeting
27018:00 <bountiful> thanks glozow
27118:00 <michaelfolkson> Thanks glozow!
27218:00 <AnthonyRonning> thank you glozow and everyone else!
27318:00 <jnewbery> Thanks for hosting glozow. That was great!
27418:00 <lightlike> thanks glozow!
27518:00 <stacie> thanks glozow! learned a lot ⭐️
27618:00 <robot-dreams> Thanks glozow!
27718:00 <thomasb06> thanks glozow
27818:00 <ariard> thanks glozow!
27918:00 <AnthonyRonning> Good work on the PR and +1 for commit structure too, made it easy to read
28018:00 <dhruvm> jnewbery: thank you glozow !
28118:00 <norisgOG> thank you glozow
28218:01 <n3wBEEEE> thanks
28318:01 <michaelfolkson> I don't think Coinjoin is a use case glozow. Did you mean Payjoin? https://brink.dev/blog/2021/01/21/fellowship-project-package-accept/
28418:01 <jnewbery> I'm hosting next week. I'll post notes and questions by Friday
28518:01 <sunon> Thanks for hosting, glozow!
28618:01 <murtyjones> thanks glozow!
28718:01 <Murch> thanks!
28818:01 <prayank> Thanks
28918:01 <svav> Thanks glozow
29018:01 <michaelfolkson> Oh no sorry not Payjoin, CoinSwap. Lol all the words
29118:02 <thomasb06> jnewbery: by the way, would it be useful for Bitcoin to have an error correction algorithm at bit level? My guess is no, but I'm trying to plug in something I know
29218:02 <michaelfolkson> I don't think Coinjoin and Payjoin have chains of multiple transactions
29318:02 <emzy> Thanks glozow!
29418:02 <michaelfolkson> And ariard if you are hanging around can we continue the conversation on use cases please? :)
29518:03 <glozow> michaelfolkson: they have transactions where you are not in full control of the inputs
29618:03 <glozow> and thus fee bumping would either require re-negotiation with possibly untrusted parties or a CPFP
29718:04 <prayank> glozow: "Critical" in my comment was based on influence and importance of a project and I had read about it here: https://twitter.com/_jonasschnelli_/status/1337351770192961540
29818:04 <ariard> michaelfolkson: a coinjoin counterparty might use its output as the starting outpoint of a new chain without other coinjoin counterparies being aware of it ?
29918:04 <robot-dreams> Following up on something I didn't understand from earlier, how come we do `ConsensusScriptChecks` in the testmempoolaccept -> AcceptSingleTransaction path, but not in the AcceptMultipleTransactions path?
30018:04 <ariard> but yeah here if you have use cases questions :)
30118:04 <michaelfolkson> glozow: So the use case is you construct a Coinjoin with a certain fee level and then the fee level ramps up straight away?
30218:04 <glozow> prayank: ah nice, thanks for linking. I just meant it as a joke since we were talking about the ubiquity of cs_main :P
30318:04 <glozow> (and thanks for waiting until after the meeting btw!)
30418:05 <glozow> robot-dreams: strictly speaking it's not necessary in the AcceptSingle with test_accept=true path
30518:05 <ariard> robot-dreams: we don't use AcceptMultipleTransactions for txn potentially included in block, as of today
30618:06 <glozow> I originally was going to call ConsensusScriptChecks in AcceptMultiple, but there was a discussion about it not being necessary (see https://github.com/bitcoin/bitcoin/pull/20833#discussion_r552997459)
30718:06 <michaelfolkson> My understanding is that the use case for package relay is where you construct a chain of transactions that you will broadcast onchain at some point in the future (days, weeks, months)
30818:06 <glozow> (I'd definitely add it back in for-realsies package accept)
30918:07 <robot-dreams> Makes sense. So we could technically only call ConsensusScriptChecks in AcceptSingle in the `if (!args.m_test_accept)` case?
31018:07 <robot-dreams> Actually is there a strong reason for that (e.g. avoid script result caching for test accepts)
31118:07 <glozow> robot-dreams: yes, it would still work :)
31218:07 <michaelfolkson> It is pretty unlikely that you'd construct a Coinjoin with a fee and then just before you broadcast it the network fees jump massively. I guess it is possible
31318:07 <glozow> robot-dreams: I agree
31418:08 <glozow> michaelfolkson: you can think of package relay being useful for fee-bumping transactions where you are not in full control of the inputs
31518:08 <michaelfolkson> Anyway ariard... you said earlier " not an issue, justice transactions are fully malleable"
31618:09 <ariard> michaelfolkson: in the context of multi-party protocol: it's either a) a conveniency mechanism is the bumped tx isn't time-sensitive or b) a security mechanism if they're
31718:09 <ariard> e.g a) is coinjoin, bumping with a package attached might save you from a new round of signatures
31818:10 <michaelfolkson> glozow: Right but the increased fee has to come from someone's input. I'd think it was better to just reconstruct a new Coinjoin
31918:11 <michaelfolkson> Something has surely gone wrong if there wasn't a large enough fee set in a Coinjoin transaction
32018:12 <michaelfolkson> I guess the coordinator of the Coinjoin could take the hit
32118:12 <glozow> let's say you constructed a CoinJoin with a medium fee, and you want it to be mined sooner
32218:12 <glozow> it hasn't left the mempool yet, so if you tried to construct a new CoinJoin it would be conflicting
32318:13 <michaelfolkson> Yeah maybe that happens. I'd have thought Coinjoins would want to confirm immediately given the number of participants and how hard it is to organize
32418:13 <glozow> you can imagine participants in multi-party transactions who have different time preference for the transaction
32518:14 <michaelfolkson> Yeah maybe... ok!
32618:14 <robot-dreams> One more follow-up from earlier, want to make sure I understand the caching / uncaching correctly.
32718:14 <michaelfolkson> ariard: " not an issue, justice transactions are fully malleable" You referring to anchor outputs here?
32818:14 <glozow> robot-dreams: hit me!
32918:15 <robot-dreams> Here's my current understanding. If we're adding a transaction to the mempool, we bring the inputs into cache, validate, then remove them (because they're spent). If we're TESTING though, we bring the inputs into cache, validate, then manually uncache them?
33018:15 <ariard> michaelfolkson: no, justice transactions have always been fully malleable by the cheated victim
33118:16 <ariard> though package relay would be useful for delegating such transactions for watchtowers
33218:16 <ariard> *to
33318:17 <robot-dreams> ariard: Earlier I wasn't referring to fee bumping a justice transaction with CPFP, I was referring to double checking that the chain of 2 transactions would be accepted together even though the commitment transaction hadn't been broadcast yet. Would that use case still make sense in the face of malleable justice transactions?
33418:18 <glozow> robot-dreams: if we added it successfully to the mempool, we keep them in cache because we expect to see them again. but if it was invalid, we uncache them.
33518:19 <michaelfolkson> What exactly is malleable in the justice transaction? It is spending outputs of the expired commitment transaction (pre-ANYPREVOUT)
33618:20 <robot-dreams> glozow: I think I have some confusion over what is "it" and "them" :-P if we add transaction T with inputs vin and outputs vout to the mempool, here's my understanding of what happens:
33718:20 <ariard> robot-dreams: I think I understand your question better, you mean being sure you can punish correctly a revoked commitment. Yes it makes sense to use testmempoolaccept when you manually assert correctness of your transaction generation code
33818:21 <robot-dreams> ariard: Thanks!
33918:21 <robot-dreams> glozow: 1. bring everything in vin into the cache, 2. validate, 3. remove everything from vin from the cache / add everything from vout to the cache, is that right?
34018:21 <ariard> but not for production as you don't have to txstandardardness-verify a counterparty conribution, all justice transaction fields are controlled by the punisher
34118:21 <glozow> oops 😂 yeah so we care about all the coins that are spent by a tx, and we don't mark them as spent if we're just adding to mempool. but we do want to keep them in the cache for later.
34218:22 <ariard> michaelfolkson: in this multi-party context, malleable == single-signed in opposition to counter-signed-and-some-field-selected-by-other
34318:22 <robot-dreams> Cool, so in the case of a TEST accept, once we've validated, we want to remove from the cache every vin that wasn't previously in it?
34418:22 <glozow> as for the coins created in the tx's vouts, those don't exist yet
34518:22 <glozow> it's not a test vs non test accept, it's a valid vs invalid thing
34618:23 <glozow> but yes otherwise that is correct
34718:24 <michaelfolkson> ariard: Hmmm ok. Different definitions of malleability in single party and multi party contexts apparently :)
34818:25 <michaelfolkson> Ok then finally your comment here ariard: https://github.com/bitcoin/bitcoin/pull/20833#pullrequestreview-570114670
34918:25 <robot-dreams> In `ProcessNewPackage` it looks like you unconditionally uncache, but in `AcceptToMemoryPoolWithTime` it looks like you only uncache in the `!result.m_accepted` case
35018:26 <robot-dreams> Am I missing something here or are you going for different semantics in the single vs. package cases?
35118:26 <glozow> OH right, so they're different between single and package yeah
35218:26 <glozow> for single, it's valid/invalid
35318:26 <glozow> for package, it's basically always
35418:26 <glozow> sorry my bad
35518:26 <michaelfolkson> ariard: You saying here that Lightning won't use package relay? Or you saying Lightning won't use testmempoolaccept?
35618:27 <robot-dreams> Cool, that makes sense! And what was the motivation for not making the valid/invalid distinction for package?
35718:27 <glozow> uh, i was scared of adding a bug
35818:28 <glozow> 😅 I think i would do it by valid/invalid in for-realsies package
35918:28 <michaelfolkson> "In most L2s, chain of transactions validity is more liberal than mempool one. Integrating this new testmempoolaccept might break it..."
36018:28 <glozow> also, i need to come up with a better name than for-realsies
36118:28 <michaelfolkson> That's interesting
36218:28 <robot-dreams> You mean "move fast and break things" is not the motto of bitcoin core? :surprised-pikachu: anyway thanks, that all makes a lot of sense
36318:28 <ariard> michaelfolkson: what LN needs is a configurable txstandardness verifier, not exactly what testmempoolaccept is for a first-throw
36418:29 <ariard> but it such configurations options can be added later
36518:29 <glozow> yeah i don't think you can test LN chains with this
36618:29 <glozow> I don't think I claimed that
36718:29 <ariard> glozow: public package, p2p-pakcages ?
36818:29 <robot-dreams> I like for-realsies, but are you going for something like "atomic" package (all or nothing)?
36918:29 <michaelfolkson> Ok I think I'm with you. I think I've caught up
37018:30 <robot-dreams> or "batch" package?
37118:30 <robot-dreams> or you troll everyone by calling it a "transactional" package cause in DB land transaction = all or nothing
37218:30 <michaelfolkson> No you haven't claimed that glozow. In my mind I had claimed that :)
37318:30 <glozow> HAHAHA
37418:30 <michaelfolkson> Incorrectly it turns out
37518:31 <glozow> michaelfolkson: well hopefully we get there, heh
37618:31 <michaelfolkson> Ok thanks for hanging around after glozow ariard
37718:31 <ariard> glozow Ahah no I don't think you claimed that, it's just "second-layer application chains" cover a bunch of different beasts in practice :)
37818:32 <glozow> whew whew
37918:32 <michaelfolkson> Yeah I'm of the strong view the focus should be on Lightning until some of the other second layers know what they need
38018:32 <michaelfolkson> Fix Lightning problems first
38118:33 <michaelfolkson> But Coinjoin isn't second layer if that turns out to be a use case
38218:33 <glozow> oh, the CoinJoin thing isn't that relevant for testmempoolaccept i think
38318:34 <michaelfolkson> So this is like a stepping stone advancing testmempoolaccept in the hope that a few more stepping stones along we'll start solving real problems
38418:34 <michaelfolkson> (with package relay)
38518:35 <michaelfolkson> Maybe testmempoolaccept doesn't have any use cases but a few more stepping stones along a use case or multiple use cases start getting solved
38618:36 <michaelfolkson> If true sounds like a good approach
38718:38 <glozow> yeah, imo that's a fine way to think about it