qa: Fix bench/block_assemble assert failure #13806

pull jamesob wants to merge 1 commits into bitcoin:master from jamesob:2018-07-assemble-block-fault changing 1 files +8 −4

jamesob commented at 3:42 PM on July 30, 2018: member

Calling bench_bitcoin currently fails due to calling ATMP without acquiring cs_main first in the recently added block_assemble bench (https://github.com/bitcoin/bitcoin/pull/13219).

$ cat <(uname -a) <(gcc --version)

Linux james 4.4.0-119-generic [#143](/bitcoin-bitcoin/143/)+jamesob SMP Mon Apr 16 21:47:24 EDT 2018 x86_64 x86_64 x86_64 GNU/Linux
gcc (Ubuntu 5.4.0-6ubuntu1~16.04.9) 5.4.0 20160609

$ ./src/bench/bench_bitcoin

WARNING: This is a debug build - may result in slower benchmarks.
# Benchmark, evals, iterations, total, min, max, median
Assertion failed: lock cs_main not held in validation.cpp:566; locks held:
[1]    19323 abort (core dumped)  ./src/bench/bench_bitcoin

(gdb) bt
[#0](/bitcoin-bitcoin/0/)  0x00007fbdc9cf5428 in __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:54
[#1](/bitcoin-bitcoin/1/)  0x00007fbdc9cf702a in __GI_abort () at abort.c:89
[#2](/bitcoin-bitcoin/2/)  0x0000555a19580dc5 in AssertLockHeldInternal (pszName=pszName@entry=0x555a19834549 "cs_main",
    pszFile=pszFile@entry=0x555a1988a001 "validation.cpp", nLine=nLine@entry=566, cs=cs@entry=0x555a19ba55c0 <cs_main>) at sync.cpp:157
[#3](/bitcoin-bitcoin/3/)  0x0000555a194b395f in AcceptToMemoryPoolWorker (chainparams=..., pool=..., state=...,
    ptx=std::shared_ptr (count 1, weak 0) 0x555a1bb819b0, pfMissingInputs=pfMissingInputs@entry=0x0, nAcceptTime=1532964079,
    plTxnReplaced=0x0, bypass_limits=false, nAbsurdFee=@0x7ffcbc1719d8: 0, coins_to_uncache=std::vector of length 0, capacity 0,
    test_accept=false) at validation.cpp:566
[#4](/bitcoin-bitcoin/4/)  0x0000555a194ba661 in AcceptToMemoryPoolWithTime (chainparams=..., pool=..., state=...,
    tx=std::shared_ptr (count 1, weak 0) 0x555a1bb819b0, pfMissingInputs=pfMissingInputs@entry=0x0, nAcceptTime=<optimized out>,
    plTxnReplaced=0x0, bypass_limits=false, nAbsurdFee=0, test_accept=false) at validation.cpp:998
[#5](/bitcoin-bitcoin/5/)  0x0000555a194ba7ce in AcceptToMemoryPool (pool=..., state=..., tx=std::shared_ptr (count 1, weak 0) 0x555a1bb819b0,
    pfMissingInputs=pfMissingInputs@entry=0x0, plTxnReplaced=plTxnReplaced@entry=0x0, bypass_limits=bypass_limits@entry=false, nAbsurdFee=0,
    test_accept=false) at validation.cpp:1014
[#6](/bitcoin-bitcoin/6/)  0x0000555a19363fbe in AssembleBlock (state=...) at bench/block_assemble.cpp:102
[#7](/bitcoin-bitcoin/7/)  0x0000555a193654d3 in std::_Function_handler<void (benchmark::State&), void (*)(benchmark::State&)>::_M_invoke(std::_Any_data const&, benchmark::State&) (__functor=..., __args#0=...) at /usr/include/c++/5/functional:1871
[#8](/bitcoin-bitcoin/8/)  0x0000555a193501d7 in std::function<void (benchmark::State&)>::operator()(benchmark::State&) const (this=this@entry=0x555a1ba2cda0,
    __args#0=...) at /usr/include/c++/5/functional:2267
[#9](/bitcoin-bitcoin/9/)  0x0000555a1934ec4c in benchmark::BenchRunner::RunAll (printer=..., num_evals=5, scaling=<optimized out>, filter=..., is_list_only=false)
    at bench/bench.cpp:121
[#10](/bitcoin-bitcoin/10/) 0x0000555a1934ade9 in main (argc=<optimized out>, argv=<optimized out>) at bench/bench_bitcoin.cpp:92

in src/validation.cpp:996 in 5973a745e4 outdated

 992 | @@ -993,6 +993,7 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool
 993 |  static bool AcceptToMemoryPoolWithTime(const CChainParams& chainparams, CTxMemPool& pool, CValidationState &state, const CTransactionRef &tx,
 994 |                          bool* pfMissingInputs, int64_t nAcceptTime, std::list<CTransactionRef>* plTxnReplaced,
 995 |                          bool bypass_limits, const CAmount nAbsurdFee, bool test_accept)
 996 | +                        EXCLUSIVE_LOCKS_REQUIRED(cs_main)

MarcoFalke commented at 3:50 PM on July 30, 2018:

This opens up a can of worms, better leave it for another time.

jamesob commented at 3:54 PM on July 30, 2018:

Fixed.

in src/bench/block_assemble.cpp:102 in 5973a745e4 outdated
```
  98 | @@ -99,6 +99,7 @@ static void AssembleBlock(benchmark::State& state)
  99 |      }
 100 |      for (const auto& txr : txs) {
 101 |          CValidationState state;
 102 | +        LOCK(::cs_main);
```
MarcoFalke commented at 3:52 PM on July 30, 2018:

Could just take the lock in the first line of the benchmark once?

jamesob commented at 3:54 PM on July 30, 2018:

Fixed.
jamesob force-pushed on Jul 30, 2018
MarcoFalke renamed this:
~~Fix bench/block_assemble assert failure~~
qa: Fix bench/block_assemble assert failure
on Jul 30, 2018
MarcoFalke added the label Tests on Jul 30, 2018
jamesob force-pushed on Jul 30, 2018
Acquire cs_main before ATMP call in block_assemble bench
Otherwise we fail an assert in sync.cpp:AssertLockHeldInternal.
6f53edb395
jamesob force-pushed on Jul 30, 2018
MarcoFalke commented at 4:15 PM on July 30, 2018: member

utACK 6f53edb395

I hope we have the compile time lock annotations soon, so we are no longer running into these run time issues in the future.
MarcoFalke merged this on Jul 30, 2018
MarcoFalke closed this on Jul 30, 2018
MarcoFalke referenced this in commit 4d550ffab6 on Jul 30, 2018
deadalnix referenced this in commit 4de1bab17a on Apr 27, 2020
ftrader referenced this in commit db6ebe7f65 on Aug 17, 2020
MarcoFalke locked this on Sep 8, 2021

Contributors

jamesob

MarcoFalke

Labels

Tests

Linked (view graph)

#1 JSON-RPC support for mobile devices ("ultra-lightweight" clients)#2 Long-term, safe, store-of-value #3 Encrypt wallet #4 Export/Import wallet in a human readable, future-proof format #5 Make the version number the protocol version and not the client version #6 Treat wallet as a generic keystore #7 Block-header-only, faster startup client #8 RPC command to sign text with wallet private key #9 Fix for GUI on Macs and latest wxWidgets #10 Add address to listtransactions output #143 Fix deadlocks in getaccountaddress, setaccount, sendfrom RPC calls #13219 bench: Add block assemble benchmark