mempool: Stricter orphan evaluation and eviction. #1207

dnldd · 2018-05-14T09:54:11Z

This PR contains the following upstream commits:

70db324

This modifies the way orphan removal and processing is done to more
aggressively remove orphans that can no longer be valid due to other
transactions being added or removed from the primary transaction pool.

The net effect of these changes is that orphan pool will typically be
much smaller which greatly improves its effectiveness. Previously, it
would typically quickly reach the max allowed worst-case usage and
effectively stay there forever.

The following is a summary of the changes:

Modify the map that tracks which orphans redeem a given transaction to
instead track by the specific outpoints that are redeemed
Modify the various orphan removal and processing functions to accept
the full transaction rather than just its hash
Introduce a new flag on removeOrphans which specifies whether or not
to remove the transactions that redeem the orphan being removed as
well which is necessary since only some paths require it
Add a new function named removeOrphanDoubleSpends that is invoked
whenever a transaction is added to the main pool and thus the outputs
they spent become concrete spends
Introduce a new flag on maybeAcceptTransaction which specifies whether
or not duplicate orphans should be rejected since only some paths
require it
Modify processOrphans as follows:
- Make use of the modified map
- Use newly available flags and logic work more strictly work with tx
  chains
- Recursively remove any orphans that also redeem any outputs redeemed
  by the accepted transactions
Several new tests to ensure proper functionality
- Removing an orphan that doesn't exist is removed both when there is
  another orphan that redeems it and when there is not
- Removing orphans works properly with orphan chains per the new
  remove redeemers flag
- Removal of multi-input orphans that double spend an output when a
  concrete redeemer enters the transaction pool

Ticket purchase, vote and revocation orphan tests have also been added.

This modifies the way orphan removal and processing is done to more aggressively remove orphans that can no longer be valid due to other transactions being added or removed from the primary transaction pool. The net effect of these changes is that orphan pool will typically be much smaller which greatly improves its effectiveness. Previously, it would typically quickly reach the max allowed worst-case usage and effectively stay there forever. The following is a summary of the changes: - Modify the map that tracks which orphans redeem a given transaction to instead track by the specific outpoints that are redeemed - Modify the various orphan removal and processing functions to accept the full transaction rather than just its hash - Introduce a new flag on removeOrphans which specifies whether or not to remove the transactions that redeem the orphan being removed as well which is necessary since only some paths require it - Add a new function named removeOrphanDoubleSpends that is invoked whenever a transaction is added to the main pool and thus the outputs they spent become concrete spends - Introduce a new flag on maybeAcceptTransaction which specifies whether or not duplicate orphans should be rejected since only some paths require it - Modify processOrphans as follows: - Make use of the modified map - Use newly available flags and logic work more strictly work with tx chains - Recursively remove any orphans that also redeem any outputs redeemed by the accepted transactions - Several new tests to ensure proper functionality - Removing an orphan that doesn't exist is removed both when there is another orphan that redeems it and when there is not - Removing orphans works properly with orphan chains per the new remove redeemers flag - Removal of multi-input orphans that double spend an output when a concrete redeemer enters the transaction pool

davecgh

There are several cases where this is incorrect in terms of the tree. It is creating prevOut without regard to the appropriate tree which means it will not properly detect stake transactions. Tests for stake transactions need to be added as well to prove correctness.

davecgh · 2018-07-27T23:29:43Z

mempool/mempool.go

 				make(map[chainhash.Hash]*dcrutil.Tx)
+			log.Infof("added orphans by pre entry for %v", txIn.PreviousOutPoint)


Looks like leftover debug.

davecgh · 2018-07-27T23:30:20Z

mempool/mempool.go

+
+	// Remove any orphans that redeem outputs from this one if requested.
+	if removeRedeemers {
+		prevOut := wire.OutPoint{Hash: *txHash}


This section is not correct for Decred since it ignores the Tree. It will miss stake transactions which have a Tree of wire.TxTreeStake.

davecgh · 2018-08-01T05:50:56Z

mempool/mempool.go

+		}
+
+		for txOutIdx := range tx.MsgTx().TxOut {
+			prevOut := wire.OutPoint{Hash: *txHash,


This should be outside of the loop. No need to create another struct each iteration, just update the index.

prevOut := wire.OutPoint{Hash: *txHash, Tree: tree} for txOutIdx := range tx.MsgTx().TxOut { prevOut.Index = uint32(txOutIdx) for _, orphan := range mp.orphansByPrev[prevOut] { mp.removeOrphan(orphan, true) } }

davecgh · 2018-08-01T06:17:09Z

mempool/mempool.go

-			if txRedeemer, exists := mp.outpoints[*outpoint]; exists {
+
+		for i := uint32(0); i < uint32(len(tx.MsgTx().TxOut)); i++ {
+			prevOut := wire.OutPoint{Hash: *txHash, Index: i, Tree: tree}


Same here regarding the outpoint outside of the loop.

davecgh · 2018-08-01T06:21:53Z

mempool/mempool.go

-				log.Debugf("Unable to move orphan transaction "+
-					"%v to mempool: %v", tx.Hash(), err)
+			// Skip to the next available output if there are none.
+			prevOut := wire.OutPoint{Hash: *processItem.Hash(),


Same here. prevOut should be outside the loop without Index specified and set .Index inside the loop.

davecgh · 2018-08-01T06:25:57Z

mempool/mempool_test.go

@@ -14,6 +15,12 @@ import (
 	"testing"
 	"time"

+	"github.com/decred/dcrd/chaincfg/chainec"
+


extra blank line

davecgh · 2018-08-01T06:26:05Z

mempool/mempool_test.go

+	"github.com/decred/dcrd/blockchain/chaingen"
+	"github.com/decred/dcrd/blockchain/stake"
+	"github.com/decred/dcrd/dcrec"
+


extra blank line

davecgh · 2018-08-01T08:27:46Z

mempool/mempool_test.go

+		t.Fatalf("ProcessTransaction: failed to accept valid "+
+			"transaction %v", err)
+	}
+


Extra blank line.

davecgh · 2018-08-01T08:27:50Z

mempool/mempool_test.go

+		t.Fatalf("ProcessTransaction: failed to accept orphan "+
+			"transaction %v", err)
+	}
+


Extra blank line.

davecgh · 2018-08-01T08:28:13Z

mempool/mempool_test.go

+	// utxos can now be found.
+	_, err = harness.txPool.ProcessTransaction(revocation, false, false, true)
+	if err != nil {
+		t.Fatalf("ProcessTransaction: failed to accept orphan "+


davecgh · 2018-08-01T08:28:26Z

mempool/mempool_test.go

+
+	// Generate fake mined utxos for the regular transaction and the
+	// ticket created.
+	harness.AddFakeUTXO(tx, int64(tx.MsgTx().TxIn[0].BlockHeight))


This isn't needed. Only the ticket below.

davecgh · 2018-08-01T08:29:37Z

mempool/mempool_test.go

+func (p *poolHarness) CreateVote(ticket *dcrutil.Tx) (*dcrutil.Tx, error) {
+	// Calculate the vote subsidy
+	subsidy := blockchain.CalcStakeVoteSubsidy(p.txPool.cfg.SubsidyCache,
+		p.chain.BestHeight(),


Other param should be on the same line.

davecgh

Code all looks correct now, but the recent update lost the upstream merge.

This modifies the way orphan removal and processing is done to more aggressively remove orphans that can no longer be valid due to other transactions being added or removed from the primary transaction pool. The net effect of these changes is that orphan pool will typically be much smaller which greatly improves its effectiveness. Previously, it would typically quickly reach the max allowed worst-case usage and effectively stay there forever. The following is a summary of the changes: - Modify the map that tracks which orphans redeem a given transaction to instead track by the specific outpoints that are redeemed - Modify the various orphan removal and processing functions to accept the full transaction rather than just its hash - Introduce a new flag on removeOrphans which specifies whether or not to remove the transactions that redeem the orphan being removed as well which is necessary since only some paths require it - Add a new function named removeOrphanDoubleSpends that is invoked whenever a transaction is added to the main pool and thus the outputs they spent become concrete spends - Introduce a new flag on maybeAcceptTransaction which specifies whether or not duplicate orphans should be rejected since only some paths require it - Modify processOrphans as follows: - Make use of the modified map - Use newly available flags and logic work more strictly work with tx chains - Recursively remove any orphans that also redeem any outputs redeemed by the accepted transactions - Several new tests to ensure proper functionality - Removing an orphan that doesn't exist is removed both when there is another orphan that redeems it and when there is not - Removing orphans works properly with orphan chains per the new remove redeemers flag - Removal of multi-input orphans that double spend an output when a concrete redeemer enters the transaction pool Ticket purchase, vote and revocation orphan tests have also been added.

davecgh added this to the 1.3.0 milestone May 14, 2018

davecgh requested changes Jul 1, 2018

View reviewed changes

dnldd force-pushed the merge_mempool_orphan_outpoint branch 4 times, most recently from 4f8f72e to 6ab2609 Compare July 9, 2018 17:01

davecgh requested changes Jul 27, 2018

View reviewed changes

dnldd force-pushed the merge_mempool_orphan_outpoint branch from 871e445 to fb05050 Compare July 28, 2018 17:35

davecgh requested changes Aug 1, 2018

View reviewed changes

dnldd force-pushed the merge_mempool_orphan_outpoint branch 2 times, most recently from a8182a0 to 0cfd713 Compare August 1, 2018 14:33

davecgh approved these changes Aug 1, 2018

View reviewed changes

dnldd force-pushed the merge_mempool_orphan_outpoint branch from 0cfd713 to e321244 Compare August 1, 2018 17:33

davecgh requested changes Aug 1, 2018

View reviewed changes

dnldd force-pushed the merge_mempool_orphan_outpoint branch 3 times, most recently from f7fbb59 to a21705a Compare August 1, 2018 18:11

dnldd force-pushed the merge_mempool_orphan_outpoint branch from a21705a to e488d41 Compare August 1, 2018 18:57

davecgh approved these changes Aug 1, 2018

View reviewed changes

davecgh merged commit e488d41 into decred:master Aug 1, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mempool: Stricter orphan evaluation and eviction. #1207

mempool: Stricter orphan evaluation and eviction. #1207

dnldd commented May 14, 2018 •

edited

davecgh left a comment

davecgh Jul 27, 2018

davecgh Jul 27, 2018 •

edited

davecgh Aug 1, 2018

davecgh Aug 1, 2018

davecgh Aug 1, 2018

davecgh Aug 1, 2018

davecgh Aug 1, 2018

davecgh Aug 1, 2018

davecgh Aug 1, 2018

davecgh Aug 1, 2018

davecgh Aug 1, 2018

davecgh Aug 1, 2018

davecgh left a comment

		make(map[chainhash.Hash]*dcrutil.Tx)
		log.Infof("added orphans by pre entry for %v", txIn.PreviousOutPoint)

mempool: Stricter orphan evaluation and eviction. #1207

mempool: Stricter orphan evaluation and eviction. #1207

Conversation

dnldd commented May 14, 2018 • edited

davecgh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davecgh Jul 27, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davecgh left a comment

Choose a reason for hiding this comment

dnldd commented May 14, 2018 •

edited

davecgh Jul 27, 2018 •

edited