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

Fix possible lock contention in omni_getactivedexsells #465

Merged
merged 1 commit into from Apr 8, 2017

Conversation

Projects
None yet
2 participants
@zathras-crypto
Copy link

zathras-crypto commented Apr 7, 2017

This PR fixes a rare DExv1 bug that can cause lock contention when a new block is connected while the omni_getactivedexsells RPC is processing accept offers.

This has only been seen in the wild at Omni Explorer (where the RPCs are hit very frequently) AFAIK.

For verification, the following regtest script will simulate the mining of blocks and the calling of omni_getactivedexsells at the same time with high frequency to trigger this bug, and will reliably crash an Omni Core node that does not have the fix noted in this PR.

Thanks
Z

#!/bin/bash

SRCDIR=./src/
NUL=/dev/null
PASS=0
FAIL=0
clear
printf "Preparing a test environment...\n"
printf "   * Starting a fresh regtest daemon\n"
rm -r ~/.bitcoin/regtest
$SRCDIR/omnicored --regtest --server --daemon >$NUL
sleep 3
printf "   * Preparing some mature testnet BTC\n"
$SRCDIR/omnicore-cli --regtest generate 102 >$NUL
printf "   * Obtaining a master address to work with\n"
ADDR=$($SRCDIR/omnicore-cli --regtest getnewaddress OMNIAccount)
printf "   * Funding the address with some testnet BTC for fees\n"
$SRCDIR/omnicore-cli --regtest sendtoaddress $ADDR 20 >$NUL
$SRCDIR/omnicore-cli --regtest generate 1 >$NUL
ADDR2=$($SRCDIR/omnicore-cli --regtest getnewaddress ACCEPTAccount)
printf "   * Participating in the Exodus crowdsale to obtain some OMNI\n"
JSON="{\"moneyqMan7uh8FqdCA2BV5yZ8qVrc9ikLP\":10,\""$ADDR"\":4,\""$ADDR2"\":2}"
$SRCDIR/omnicore-cli --regtest sendmany OMNIAccount $JSON >$NUL
$SRCDIR/omnicore-cli --regtest generate 1 >$NUL
printf "   * Creating a sell offer from the master address\n"
$SRCDIR/omnicore-cli --regtest omni_senddexsell $ADDR 1 "11.5" "0.75" 25 "0.0005" 1 >$NUL
$SRCDIR/omnicore-cli --regtest generate 1 >$NUL
printf "   * Accepting the sell offer\n"
$SRCDIR/omnicore-cli --regtest omni_senddexaccept $ADDR2 $ADDR 1 10 >$NUL
$SRCDIR/omnicore-cli --regtest generate 1 >$NUL
printf "\nNow spawning a separate thread to call omni_getactivedexsells in a loop, while we mine blocks here\n"
printf "There should be 100x 'GET' and 100x 'MINE' events, followed by a 'Reached the end of the test' note\n"
printf "Starting...\n"
loopscript=`mktemp /tmp/.script.XXXXXX`;
cat >$loopscript <<END
for i in {1..100}
do
  printf "GET,"
  ./src/omnicore-cli --regtest omni_getactivedexsells >$NUL
done
END
chmod u+rx $loopscript
$loopscript &
for i in {1..100}
do
  printf "MINE,"
  $SRCDIR/omnicore-cli --regtest generate 1 >$NUL
done
wait
/bin/rm $loopscript
printf "\nReached the end of the test.\n"
$SRCDIR/omnicore-cli --regtest stop
exit

@dexX7 dexX7 modified the milestone: Next release Apr 8, 2017

@dexX7 dexX7 merged commit c79a7c9 into OmniLayer:develop Apr 8, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

dexX7 added a commit that referenced this pull request Apr 8, 2017

Merge #465: Fix possible lock contention in omni_getactivedexsells
c79a7c9 Move request for GetHeight() out of cs_tally lock in omni_getactivedexsells (Zathras Crypto)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment