qa: Fix bench/block_assemble assert failure #13806

jamesob · 2018-07-30T15:42:54Z

Calling bench_bitcoin currently fails due to calling ATMP without acquiring cs_main first in the recently added block_assemble bench (#13219).

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

Linux james 4.4.0-119-generic #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  0x00007fbdc9cf5428 in __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:54
#1  0x00007fbdc9cf702a in __GI_abort () at abort.c:89
#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  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  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  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  0x0000555a19363fbe in AssembleBlock (state=...) at bench/block_assemble.cpp:102
#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  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  0x0000555a1934ec4c in benchmark::BenchRunner::RunAll (printer=..., num_evals=5, scaling=<optimized out>, filter=..., is_list_only=false)
    at bench/bench.cpp:121
#10 0x0000555a1934ade9 in main (argc=<optimized out>, argv=<optimized out>) at bench/bench_bitcoin.cpp:92

maflcko · 2018-07-30T15:50:12Z

src/validation.cpp

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


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

maflcko · 2018-07-30T15:52:07Z

src/bench/block_assemble.cpp

@@ -99,6 +99,7 @@ static void AssembleBlock(benchmark::State& state)
    }
    for (const auto& txr : txs) {
        CValidationState state;
+        LOCK(::cs_main);


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

Otherwise we fail an assert in sync.cpp:AssertLockHeldInternal.

maflcko · 2018-07-30T16:15:25Z

utACK 6f53edb

I hope we have the compile time lock annotations soon, so we are no longer running into these run time issues in the future.

6f53edb Acquire cs_main before ATMP call in block_assemble bench (James O'Beirne) Pull request description: Calling `bench_bitcoin` currently fails due to calling ATMP without acquiring cs_main first in the recently added block_assemble bench (#13219). ``` $ cat <(uname -a) <(gcc --version) Linux james 4.4.0-119-generic #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 0x00007fbdc9cf5428 in __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:54 #1 0x00007fbdc9cf702a in __GI_abort () at abort.c:89 #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 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 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 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 0x0000555a19363fbe in AssembleBlock (state=...) at bench/block_assemble.cpp:102 #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 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 0x0000555a1934ec4c in benchmark::BenchRunner::RunAll (printer=..., num_evals=5, scaling=<optimized out>, filter=..., is_list_only=false) at bench/bench.cpp:121 #10 0x0000555a1934ade9 in main (argc=<optimized out>, argv=<optimized out>) at bench/bench_bitcoin.cpp:92 ``` Tree-SHA512: fdd7b28ff123ccea7a4f334d53f735d0c0f94aa9cc52520c2dd34dca45d78c691af64efcd32366fc472fedffbd79591d2be2bb3bfc4a5186e8712b6b452d64e3

Summary: This is a backport of Core [[bitcoin/bitcoin#13219 | PR13219]] and [[bitcoin/bitcoin#13806 | PR13806]] . Due to many backport being done out of oder or improperly, this contains elements of [[bitcoin/bitcoin#15413 | PR15413]] and [[bitcoin/bitcoin#15788 | PR15788]] Test Plan: make check ./src/bench/bench_bitcoin Check that the new benchmark is run. Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D5848

maflcko reviewed Jul 30, 2018

View reviewed changes

jamesob force-pushed the 2018-07-assemble-block-fault branch from 5973a74 to 7798bea Compare July 30, 2018 15:53

maflcko changed the title ~~Fix bench/block_assemble assert failure~~ qa: Fix bench/block_assemble assert failure Jul 30, 2018

maflcko added the Tests label Jul 30, 2018

jamesob force-pushed the 2018-07-assemble-block-fault branch from 7798bea to 18d6b4c Compare July 30, 2018 16:06

Acquire cs_main before ATMP call in block_assemble bench

6f53edb

Otherwise we fail an assert in sync.cpp:AssertLockHeldInternal.

jamesob force-pushed the 2018-07-assemble-block-fault branch from 18d6b4c to 6f53edb Compare July 30, 2018 16:08

maflcko merged commit 6f53edb into bitcoin:master Jul 30, 2018

Bushstar mentioned this pull request Aug 13, 2018

commits from bitcoin/master FeatherCoin/Feathercoin#358

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

qa: Fix bench/block_assemble assert failure #13806

qa: Fix bench/block_assemble assert failure #13806

Uh oh!

jamesob commented Jul 30, 2018 •

edited

Loading

Uh oh!

maflcko Jul 30, 2018

Uh oh!

jamesob Jul 30, 2018

Uh oh!

maflcko Jul 30, 2018

Uh oh!

jamesob Jul 30, 2018

Uh oh!

maflcko commented Jul 30, 2018

Uh oh!

Uh oh!

qa: Fix bench/block_assemble assert failure #13806

qa: Fix bench/block_assemble assert failure #13806

Uh oh!

Conversation

jamesob commented Jul 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko Jul 30, 2018

Choose a reason for hiding this comment

Uh oh!

jamesob Jul 30, 2018

Choose a reason for hiding this comment

Uh oh!

maflcko Jul 30, 2018

Choose a reason for hiding this comment

Uh oh!

jamesob Jul 30, 2018

Choose a reason for hiding this comment

Uh oh!

maflcko commented Jul 30, 2018

Uh oh!

Uh oh!

jamesob commented Jul 30, 2018 •

edited

Loading