Make mapNextTx private within CTxMemPool #5347

TheBlueMatt commented at 10:21 PM on November 21, 2014: member

This builds on #5206, removing the (now duplicate) check from AcceptToMemoryPool and makes mapNextTx private in CTxMemPool.

sipa commented at 11:35 AM on November 23, 2014: member

utACK; nice!

morcos commented at 6:04 PM on November 24, 2014: member

Sorry if I'm missing something, but why is it OK to remove the mapNextTx check? Where is the duplicate check? I ran a little test on this pull that sent a second transaction that reused an input, and the second tx was accepted to the memory pool and then caused an assertion to fail in mempool.check() at line 543 in txmempool.cpp.

sipa commented at 6:11 PM on November 24, 2014: member

@morcos In that case there is something else wrong. The first patch would avoid exposing outputs already spent inside the mempool in the used coinsview, so the "HaveInputs" check should fail for transactions that spend such outputs.

morcos commented at 6:58 PM on November 24, 2014: member

I think the problem might be that in CCoinsViewMemPool::GetCoins , mempool.pruneSpent is only called on coins returned from the mempool. But you can still return unpruned coins from the chain that have outputs spent by other transactions in the mempool.

sipa commented at 7:02 PM on November 24, 2014: member

Nice catch!

laanwj added the label Improvement on Nov 28, 2014

dgenr8 commented at 9:11 PM on November 30, 2014: contributor

Is this salvageable? If a CCoinsView is to be used for conflict checks, it would need to persist across successive incoming transactions. As @morcos says, the commits do not accomplish this.

TheBlueMatt commented at 9:18 PM on November 30, 2014: member

Sure, just need to adapt GetCoins in CCoinsViewMemPool to call pruneSpent on non-mempool coins too, easy as pie :)

dgenr8 commented at 1:55 AM on December 1, 2014: contributor

You mean start using pcoinsTip so that it no longer represents the tip, but tip+mempool?

TheBlueMatt force-pushed on Dec 21, 2014

TheBlueMatt commented at 2:10 AM on December 21, 2014: member

@dgenr8 No, like the middle commit I just pushed :p

TheBlueMatt commented at 2:44 AM on December 21, 2014: member

Also added a little mini-testcase to catch this case (and any simple mempool double-spend) just because :).

dgenr8 commented at 9:37 PM on December 21, 2014: contributor

@TheBlueMatt Ok. The mempool containing tx1 is the elusive "persistence mechanism" I was hung up on.

TheBlueMatt force-pushed on Dec 22, 2014

TheBlueMatt commented at 2:26 AM on December 22, 2014: member

@dgenr8 heh, thanks...hadnt realized this failed :)

laanwj added the label Tests on Jan 8, 2015

laanwj added the label Mempool on Jan 8, 2015

laanwj commented at 11:18 AM on March 20, 2015: member

@morcos has your comment been resolved here?

morcos commented at 2:11 PM on March 20, 2015: member

Yes that problem was fixed. But my comment on the other PR still applies in that this changes behavior. As far as I could find the only change is in rpcsignrawtransaction and preventing signing of double spends (now also when the inputs are in the chain) and maybe that's ok? You can see this by switching the order of the tx2 = ... and spend_id = ... lines in the included rpc test. Switching them will work prior to this change, but not with it.

in qa/rpc-tests/mempool_reject_doublespend.py:None in cdbeae20c1 outdated

  17 | +# Create one-input, one-output, no-fee transaction:
  18 | +class MempoolSpendCoinbaseTest(BitcoinTestFramework):
  19 | +
  20 | +    def setup_network(self):
  21 | +        # Just need one node for this test
  22 | +        args = ["-checkmempool", "-debug=mempool"]

morcos commented at 2:12 PM on March 20, 2015:

needs "-relaypriority=0" or to create the tx with a fee.

TheBlueMatt commented at 10:57 AM on March 26, 2015:

Fixed.

TheBlueMatt force-pushed on Mar 26, 2015

laanwj commented at 10:06 PM on October 1, 2015: member

Any way forward on this? Or better to close for now?

TheBlueMatt commented at 12:41 AM on October 10, 2015: member

@laanwj I'm not sure what your question was? Was this reviewable on Nov 4? Yes, unless people rejected to the change described by @morcos (no one voiced any opinion either way, AFAIR). But, yes, this now needs rebase due to mempool changes. I'll try to get to that next week or so.

CCoinsViewMemPool cleanup

Move the mempool lock into CCoinsViewMemPool, since the existing code
always manually locks the mempool (with the lock having same lifetime
as the view!).

Move a call to CMempool::pruneSpent into CCoinsViewMemPool::GetCoins,
since CCoinsViewMemPool::GetCoins is called only once in the codebase,
followed by a call to pruneSpent with a `TODO` suggesting it be moved.

93c7c3aba3

Move pruneSpent call to apply to non-mempool coins as well 4c904bf722

Remove a dup check from AcceptToMemoryPool, make mapNextTx private d3cc9f8314

Add basic mempool-reject-doublespend test 27662b4f1b

TheBlueMatt force-pushed on Nov 5, 2015

TheBlueMatt commented at 8:17 PM on November 5, 2015: member

I went ahead and rebased, in case anyone is interested in this, but the API change is maybe not great. Instead of being able to sign double-spends against unconfirmed txn freely, with this change you have to provide the inputs explicitly. Its not so hard to do, but maybe isnt such a nice API change.

TheBlueMatt closed this on Nov 5, 2015

MarcoFalke locked this on Sep 8, 2021