mempool/mining: Implement aggregate fee sorting.

[sefbkn]

Relates to #1556

This pull request modified the mining prioritization logic to sort transactions by an aggregate fee based on a transaction’s ancestors in the mempool.

Currently, the mining code prioritizes transactions by fee and only includes them in the block template if they have no dependencies. When a transaction is included in the block template, it is removed as a dependency from transactions that depend on it, making those dependent transactions eligible for block template inclusion.

Aggregating ancestor statistics for large graphs of transactions in the mempool may have high computational costs. Performing this aggregation in the block template generator would delay miners requesting a block template since all of the aggregation would need to occur for each transaction, at once. To solve this problem, the complexity is spread over time such that the transaction statistics are updated with mempool event hooks. Since the mempool is not locked during the entirety of template generation, a mining view interface is exposed to safely interact with a snapshot of the mempool.

The biggest risk in terms of performance are reorgs where transactions with many descendants are added back to the mempool. The number of transactions added to the mempool in this way is limited by the block size and proof of work, but the mempool has no restrictions on the number of ancestors tracked for a given transaction.

A potential - and not implemented - solution to this reorg problem would be to establish a limit on the number of ancestors a transaction may have in the mempool.

[davecgh]

Nice! This will obviously need a lot of review and testing, but the idea is sound and I'm happy to see work towards this.

[davecgh]

Hit a panic while testing this prior to review:

panic: interface conversion: interface {} is *mining.TxDesc, not *main.txPrioItem

goroutine 445 [running]:
main.(*txPriorityQueue).Push(0xc013acf980, 0xa6e500, 0xc012f668c0)
        src/github.com/decred/dcrd/mining.go:118 +0x100
container/heap.Push(0xde6760, 0xc013acf980, 0xa6e500, 0xc012f668c0)
        D:/go/src/container/heap/heap.go:53 +0x4c
main.(*BlkTmplGenerator).NewBlockTemplate(0xc012f66b40, 0xde44a0, 0xc00001a7e0, 0xd14eb0, 0xc0151fbf60, 0x42fd06)
        src/github.com/decred/dcrd/mining.go:1513 +0x222b
main.(*BgBlkTmplGenerator).genTemplateAsync.func1(0xc00b5c20c0, 0xde40e0, 0xc014c02340, 0x2, 0xc0004c6300)
        src/github.com/decred/dcrd/mining.go:2562 +0xac
created by main.(*BgBlkTmplGenerator).genTemplateAsync
        src/github.com/decred/dcrd/mining.go:2554 +0xff

[dnldd]

Tested this PR with dcrpool's mining harness, I'm yet to identify the root cause of the issue but I can't mine past SVH because tickets are not being bought. Switching to master fixes this issue, will take a closer look.

[davecgh]

Round 1 review. I still have quite a bit more to get through, but I figured I would get these published since they're pretty minor and you can make the changes quickly in the mean time.

[davecgh]

Also, please rebase this to the latest master. I would like to test it against the latest code and it currently has merge conflicts.

[rstaudt2]

Still need to do more reviewing/testing, but had some minor comments on the first read through.

[sefbkn]

Also, please rebase this to the latest master. I would like to test it against the latest code and it currently has merge conflicts.

This has been rebased onto master.

[rstaudt2]

Had a few more comments. Overall, I think the code updating the mining view looks pretty solid, nice work on that!

One thing to note is that I do find the logic of updating ancestorStats a bit hard to follow due to the combination of 1) several different code paths either recalculating, incrementing, or decrementing the stats and 2) that the aggregation is happening in the context of recursive functions. It may be worth considering if there is a more straightforward way to do this, otherwise, I think adding in some additional tests around some of the other scenarios that aren't currently tested (had an inline comment for these) would be helpful.

On the mining side, I did some basic tests, but need to still test that further since its a bit harder to hit all of the edge cases since there are not really any unit tests in place for that currently.

[rstaudt2]

Noticed a couple more things when reviewing the mining code. Still need to test more and try to hit all/most of the various scenarios in simnet (if you already have any instructions on how to manually setup/test some of this in simnet, that would be helpful).

[sefbkn]

Given the size of this PR and the difficulty of exercising the test cases around the paths around ancestor aggregation during template gen, I think trimming this PR back to an earlier point (last week) would be best at this point:

• Full mining view implementation in the mempool w/ tests.
• Mining prioritization uses TxMiningView.Children() to prioritize transactions without dependencies.
• Remove the usage of ancestors during block template gen.

Plus

• Incorporating the rest of the feedback around the mining view and tests.
• Rebase + squash when needed to clear up commits.

And the major changes to the template gen should get split out into another PR and addressed at a later time. Does this seem fair?

I'm really not clear on what minimum functionality is or has been expected to get this PR merged since there's been little chatter on code that's sitting here for almost a year, until now.

[davecgh]

I'll be giving this a final review and doing some more testing later today, but based on the updates, my previous review, and the fact I have been running the code without issue (with template generation active so it is being exercised of course), I believe this is probably pretty close to ready to go in.

To answer your question, the key points for me to merge this now are:

• It works as intended, perhaps most importantly, including when given unexpected data
• It does not introduce any new vulnerabilities or horrible performance regressions

[rstaudt2]

This is looking really close to me as well. I'm going to finish testing on the mining side by later today.

[rstaudt2]

Tested on both testnet and simnet, including with dependent transactions in the mempool, and things were working as expected. Had one additional clarification around the logic for max sigops per block, which seems off to me.

[davecgh]

Really nice job overall! Thanks for taking this on and producing good quality code with solid tests for the mempool implementation. This is a great addition. I spotted a few minor issues that I've identified inline. Once those are remedied (along with the few remaining other review comments, this will be good to merge.

I should note that there are some other nitpicks that I didn't call out such as that I think the graph implementation could be improved in terms of memory efficiency in a few spots and it ideally would avoid recursion in the depth-first logic, but those are not things this PR should be held up for since it is still quite reasonably efficient, works properly (outside of the aforementioned minor points), and has been heavily tested with the existing code (and suggested modifications plus sigop counting fix applied locally).

I added some timing prints and ran this on mainnet for a while to get a feel for the overall time it takes to build a template and, as can be seen, the times are quite reasonable. There is a lot of optimization work that can be done on the mining template code now that the mining module is separate and internal and therefore can have proper tests added for it.

MINR: Time to create template: 995µs
MINR: Time to create template: 2.9956ms
MINR: Time to create template: 5.9869ms
MINR: Time to create template: 3.0019ms
MINR: Time to create template: 2.9986ms
MINR: Time to create template: 4.9793ms
MINR: Time to create template: 3.001ms
MINR: Time to create template: 4.0013ms

[davecgh]

Thanks for the updates. Review of updates inline.

[davecgh]

I'm going to do some final testing with the latest updates, but barring any issues in testing, this looks ready to me.

[davecgh]

Also, just an FYI that I'm going to be squashing this down to a single commit when merging to get rid of all of the noise in the review updates.

[davecgh]

Looks like there is an issue with the ordering. I wrote a diff to make it easy to reproduce via simnet.

1. Modify the simnet harness with the following diff:

dcr_tmux_simnet_setup.sh diff

diff --git a/contrib/dcr_tmux_simnet_setup.sh b/contrib/dcr_tmux_simnet_setup.sh
index bdc463a5..6c95a5b2 100755
--- a/contrib/dcr_tmux_simnet_setup.sh
+++ b/contrib/dcr_tmux_simnet_setup.sh
@@ -58,8 +58,9 @@ rpcpass=${RPCPASS}
 simnet=1
 logdir=./log
 datadir=./data
-debuglevel=TXMP=debug,MINR=debug
+debuglevel=TXMP=debug,MINR=trace
 txindex=1
+blockprioritysize=0
 EOF

 cat > "${NODES_ROOT}/dcrctl.conf" <<EOF
@@ -176,6 +177,45 @@ fi
 EOF
 chmod +x "${NODES_ROOT}/${PRIMARY_WALLET_NAME}/xfer"

+cat > "${NODES_ROOT}/${PRIMARY_WALLET_NAME}/xferchain" <<EOF
+#!/usr/bin/env bash
+NUM=1
+case \$1 in
+  ''|*[!0-9]*) ;;
+  *) NUM=\$1 ;;
+esac
+
+utxos=\$(./ctl listunspent | jq "map(select(.spendable==true))")
+numutxos=\$(echo \$utxos | jq 'length')
+[[ \${numutxos} -ge 1 ]] || { echo "no unspent outputs"; exit 1; }
+
+utxo=\$(echo "\$utxos" | jq ".[0]")
+txid=\$(echo \$utxo | jq -r '.txid')
+vout=\$(echo \$utxo | jq -r '.vout')
+tree=\$(echo \$utxo | jq -r '.tree')
+amt=\$(echo \$utxo | jq -r '.amount')
+addr=\$(./ctl getnewaddress)
+basefee=2970
+for i in \$(seq 1 1 \${NUM}); do
+  fee=\$((\${basefee} * \${i}))
+  sendamt=\$(jq -rn "\${amt}-(\${fee}/1e8)")
+  newtxid=\$(./ctl createrawtransaction "[{\"amount\":\${amt},\"txid\":\"\${txid}\",\"vout\":\${vout},\"tree\":\${tree}}]" \
+     "{\"\${addr}\":\${sendamt}}" | \
+     ./ctl signrawtransaction - | jq -r .hex | \
+     ./ctl sendrawtransaction -)
+
+  echo "\${newtxid} (fee: \${fee}, spends from \${txid})"
+  txid=\${newtxid}
+  amt=\${sendamt}
+  vout=0
+  tree=0
+done
+cd ../dcrd1
+./ctl regentemplate
+EOF
+chmod +x "${NODES_ROOT}/${PRIMARY_WALLET_NAME}/xferchain"
+
+
 sleep 1
 tmux send-keys "./ctl importprivkey ${TSPEND_PRIMARY_WIF} imported false; ./ctl importprivkey ${TSPEND_SECONDARY_WIF} imported false" C-m

2. Start the simnet harness with the aforementioned modification:

$ ./dcr_tmux_simnet_setup.sh 

3. Switch to the dcrwallet1 window (ctrl+B 1) and run the new script to create a chain of three transactions each spending from the previous and with a higher fee:

$ ./xferchain 3

4. Switch back to the dcrd1 window (ctrl+B 0) and observe the dcrd log output in the upper pane to notice the error. It will look something like this:

2020-10-14 01:01:25.817 [DBG] MINR: Considering 3 transactions for inclusion to new block
2020-10-14 01:01:25.817 [TRC] MINR: Priority queue len 3, dependers len 2
2020-10-14 01:01:25.817 [TRC] MINR: Skipping tx 849f23d7e8ad5e3737cd01e6f91cc9d45143b377ac7c0ca463d3c69ae93fb8ad due to error in CheckTransactionInputs: output 30de87e66a07813b86ef93f9e1ee1f4c7814b94ada45583cfaa24f566e228942:0 referenced from transaction 849f23d7e8ad5e3737cd01e6f91cc9d45143b377ac7c0ca463d3c69ae93fb8ad:0 either does not exist or has already been spent
2020-10-14 01:01:25.817 [TRC] MINR: Adding tx 30de87e66a07813b86ef93f9e1ee1f4c7814b94ada45583cfaa24f566e228942 (priority 10000000000.00, feePerKB 13750.00)
2020-10-14 01:01:25.818 [DBG] MINR: Created new block template (2 transactions, 1 stake transactions, 0 treasury transactions, 2970 in fees, 2 signature operations, 539 bytes, target difficulty 0666660000000000000000000000000000000000000000000000000000000000, stake difficulty 0.0002)

[sefbkn]

Looks like there is an issue with the ordering. I wrote a diff to make it easy to reproduce via simnet.

Thank you, this is fixed.

[davecgh]

Reviewed the latest updates and confirmed all remaining known issues are resolved. I ran a full set of tests with transactions spending from each other in various ways and manually examining the block templates for correctness.

[rstaudt2]

Looks good with the latest updates. Great work on this!

[davecgh]

@sefbkn Would you please update the PR description to match reality? I was working to squash it all down and noticed the description is no longer entirely accurate.