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

multi: Add ticket exhaustion check. #2398

Merged
merged 2 commits into from
Oct 6, 2020

Conversation

davecgh
Copy link
Member

@davecgh davecgh commented Oct 5, 2020

This is an alternative approach to #2090 which resolves several of the issues it had and is originally based of an idea mentioned by @matheusd in #2047.


This modifies the mining template creation logic to prevent creation of templates that would result in an unrecoverable chain due to inevitable ticket exhaustion.

It is split into two individual commits as follows:

The first commit adds a new function named checkTicketExhaustion to blockchain which will return a new rule error if extending a given block with another block that contains a specified number of tickets will result in an unrecoverable chain due to inevitable ticket exhaustion along with tests to ensure proper functionality.

The approach taken is such that it should be easy to use in a future consensus change gated behind a vote if that is ultimately desired, which I personally think would be a good idea.

An exported variant is also provided that takes the hash of the block to extend for use by external callers such as the mining template code.

The second commit modifies the mining template creation logic to make use of the new ability along with returning an associated new mining rule error.

@davecgh davecgh added this to the 1.6.0 milestone Oct 5, 2020
@davecgh
Copy link
Member Author

davecgh commented Oct 5, 2020

Testing this might be a little bit challenging for the uninitiated, so here is a guide:

  1. Modify the simnet harness to disable automatic ticket purchases
harness script diff
diff --git a/contrib/dcr_tmux_simnet_setup.sh b/contrib/dcr_tmux_simnet_setup.sh
index bdc463a5..78fc11d8 100755
--- a/contrib/dcr_tmux_simnet_setup.sh
+++ b/contrib/dcr_tmux_simnet_setup.sh
@@ -133,7 +133,7 @@ tmux resize-pane -D 15
 tmux send-keys "cd ${NODES_ROOT}/${PRIMARY_WALLET_NAME}" C-m
 tmux send-keys "echo \"${WALLET_CREATE_CONFIG}\" | dcrwallet -C ../wallet.conf --create; tmux wait-for -S ${PRIMARY_WALLET_NAME}" C-m
 tmux wait-for ${PRIMARY_WALLET_NAME}
-tmux send-keys "dcrwallet -C ../wallet.conf --enableticketbuyer --ticketbuyer.limit=10" C-m
+tmux send-keys "dcrwallet -C ../wallet.conf" C-m
 tmux select-pane -t 1
 tmux send-keys "cd ${NODES_ROOT}/${PRIMARY_WALLET_NAME}" C-m
  1. Start the simnet harness with the aforementioned modification:
$ ./dcr_tmux_simnet_setup.sh 
  1. Mine to height 126 on dcrd1:
$ ./mine 94
  1. Notice the dcrd debug log starts complaining that extending the block with fewer than 5 ticket purchases would result in an unrecoverable chain
  1. Purchase 5 tickets with wallet1 and notice the block template is then generated as expected back in the dcrd1 log NOTE: [1]
$ ./directtickets 5

NOTE: [1]: Unfortunately, it's not possible to use the regular tickets script in the harness to recover because those are purchased via a split transaction which has to be mined first and the chain can't continue in order to mine that transaction. As a workaround, use this directtickets script which is a quick and dirty script I threw together specifically to test this and purchases the tickets directly without a split transaction.

directtickets script
#!/usr/bin/env bash
NUM=1
case $1 in
  ''|*[!0-9]*) ;;
  *) NUM=$1 ;;
esac

stakediff=$(./ctl getstakedifficulty | jq '.current * 1e8 | round')
utxos=$(./ctl listunspent | jq "map(select(.spendable==true and (.amount*1e8 | round)>${stakediff}))")
numutxos=$(echo $utxos | jq 'length')
[[ ${numutxos} -ge ${NUM} ]] || { echo "not enough unspent outputs"; exit 1; }

votingaddr=$(./ctl getnewaddress)
commitaddr=$(./ctl getnewaddress)
fee=2970
for i in $(seq 1 1 ${NUM}); do
  utxo=$(echo "$utxos" | jq ".[$((i - 1))]")
  txid=$(echo $utxo | jq -r '.txid')
  vout=$(echo $utxo | jq -r '.vout')
  tree=$(echo $utxo | jq -r '.tree')
  amt=$(echo $utxo | jq -r '.amount*1e8 | round')
  changeamt=$(jq -rn "${amt}-${stakediff}-${fee}")
  ./ctl createrawsstx "[{\"txid\":\"${txid}\",\"vout\":${vout},\"tree\":${tree},\"amt\":${amt}}]" \
    "{\"${votingaddr}\": ${stakediff}}" \
    "[{\"addr\":\"${commitaddr}\",\"commitamt\":${stakediff},\"changeaddr\":\"${commitaddr}\",\"changeamt\":${changeamt}}]" | \
    ./ctl signrawtransaction - | jq -r .hex | \
    ./ctl sendrawtransaction -
done

In order to see why this behavior is accurate, run a similar test with the current code on master without this change to actually trigger the exhaustion as follows:

  1. Start the same as above, except mine an initial 95 blocks to get to height 127 without purchasing any tickets (which is unrecoverable as will be seen)
$ ./mine 95
  1. Purchase 20 tickets directly with wallet1 so that they will be included in height 128
$ ./directtickets 20
  1. Regnerate the template on dcrd1 to ensure the tickets just purchased are included and mine another block to get to height 128:
$ ./ctl regentemplate
$ ./mine
  1. Mine 16 more blocks to attempt to allow those tickets to mature, but notice when it attempts to mine the block at height 143, it will fail with Unexpected error while processing block submitted via CPU miner: list size too small: 0 < 5 because there are no live tickets and there is no way to recover from this without a large reorganization.
$ ./mine 16

@davecgh davecgh force-pushed the multi_check_ticket_exhaustion branch from 80dcc1d to 923434c Compare October 5, 2020 06:08
@davecgh davecgh force-pushed the multi_check_ticket_exhaustion branch 3 times, most recently from 19596e8 to 19af5ba Compare October 5, 2020 11:27
Copy link
Member

@rstaudt2 rstaudt2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, excellent test cases. Tested locally as well per the instructions (thanks for adding that, definitely makes it easier to review). Noticed a couple of small typos and other than that this looks good to go to me.

blockchain/validate.go Outdated Show resolved Hide resolved
blockchain/validate_test.go Outdated Show resolved Hide resolved
blockchain/validate_test.go Outdated Show resolved Hide resolved
blockchain/validate_test.go Outdated Show resolved Hide resolved
Copy link
Member

@matheusd matheusd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified the test data and did the functional tests.

Only found a small thing for clarification.

internal/mining/mining.go Show resolved Hide resolved
@davecgh davecgh force-pushed the multi_check_ticket_exhaustion branch from 19af5ba to 00fc91b Compare October 5, 2020 17:19
This adds a new function named checkTicketExhaustion which will return
a new rule error if extending a given block with another block that contains a
specified number of tickets will result in an unrecoverable chain due to
inevitable ticket exhaustion along with tests to ensure proper
functionality.

The approach taken is such that it should be easy to use in a future
consensus change gated behind a vote if that is ultimately desired,
which I personally think would be a good idea.

An exported variant is also provided that takes the hash of the block to
extend for use by external callers such as the mining template code.
This modifies the mining template creation logic to make use of the new
ability to check if the template would result in an unrecoverable chain
due to inevitable ticket exhaustion along with an associated new mining
rule error.
@davecgh davecgh force-pushed the multi_check_ticket_exhaustion branch from 00fc91b to ccf024f Compare October 6, 2020 00:29
@davecgh davecgh merged commit ccf024f into decred:master Oct 6, 2020
@davecgh davecgh deleted the multi_check_ticket_exhaustion branch October 6, 2020 00:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants