Move CWalletDB::ReorderTransactions to CWallet #8828

pstratem commented at 4:02 PM on September 28, 2016: contributor

CWalletDB was never the right place for this.

MarcoFalke added the label Wallet on Sep 28, 2016

in src/wallet/walletdb.h:None in 653f3610b7 outdated

 154 | @@ -155,6 +155,7 @@ class CWalletDB : public CDB
 155 |  
 156 |      /// This writes directly to the database, and will not update the CWallet's cached accounting entries!
 157 |      /// Use wallet.AddAccountingEntry instead, to write *and* update its caches.
 158 | +    bool WriteAccountingEntry(const uint64_t nAccEntryNum, const CAccountingEntry& acentry);

paveljanik commented at 1:30 PM on September 30, 2016:

Move this a line down because of the comment?

paveljanik commented at 1:36 PM on September 30, 2016: contributor

test_bitcoin fails here:

Running 210 test cases...
unknown location:0: fatal error in "acc_orderupgrade": signal: illegal opcode; address of failing instruction: 0x10988a98a
wallet/test/accounting_tests.cpp:24: last checkpoint

*** 1 failure detected in test suite "Bitcoin Test Suite"
Database handles still open at environment close
Open database handle: unnamed/wallet_test.dat

laanwj commented at 4:37 PM on September 30, 2016: member

Concept ACK

laanwj commented at 5:23 PM on September 30, 2016: member

Regarding the Travis issue, I can reproduce this locally simply by running test_bitcoin:

Thread 1 "test_bitcoin" received signal SIGILL, Illegal instruction.
CWallet::ReorderTransactions (this=<optimized out>) at /.../bitcoin/bitcoin/src/wallet/wallet.cpp:681
681         nOrderPosNext = 0;
(gdb) bt
[#0](/bitcoin-bitcoin/0/)  CWallet::ReorderTransactions (this=<optimized out>) at /.../bitcoin/bitcoin/src/wallet/wallet.cpp:681
[#1](/bitcoin-bitcoin/1/)  0x00005555557c5d19 in accounting_tests::GetResults (results=std::map with 0 elements)
    at /.../bitcoin/bitcoin/src/wallet/test/accounting_tests.cpp:24
[#2](/bitcoin-bitcoin/2/)  0x00005555557c1128 in accounting_tests::acc_orderupgrade::test_method (this=<optimized out>)
    at /.../bitcoin/bitcoin/src/wallet/test/accounting_tests.cpp:58
[#3](/bitcoin-bitcoin/3/)  0x00005555557c0cde in accounting_tests::acc_orderupgrade_invoker ()
    at /.../bitcoin/bitcoin/src/wallet/test/accounting_tests.cpp:32
[#4](/bitcoin-bitcoin/4/)  0x00005555555ceb91 in boost::unit_test::ut_detail::invoker<boost::unit_test::ut_detail::unused>::invoke<void (*)()> (this=<optimized out>, 
    f=<optimized out>) at /usr/include/boost/test/utils/callback.hpp:56
[#5](/bitcoin-bitcoin/5/)  boost::unit_test::ut_detail::callback0_impl_t<boost::unit_test::ut_detail::unused, void (*)()>::invoke (this=0x0)
    at /usr/include/boost/test/utils/callback.hpp:89
[#6](/bitcoin-bitcoin/6/)  0x00007ffff70cecb1 in ?? () from /usr/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.58.0
[#7](/bitcoin-bitcoin/7/)  0x00007ffff70ae996 in boost::execution_monitor::catch_signals(boost::unit_test::callback0<int> const&) ()
   from /usr/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.58.0
[#8](/bitcoin-bitcoin/8/)  0x00007ffff70af1b3 in boost::execution_monitor::execute(boost::unit_test::callback0<int> const&) ()
   from /usr/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.58.0
[#9](/bitcoin-bitcoin/9/)  0x00007ffff70cede2 in boost::unit_test::unit_test_monitor_t::execute_and_translate(boost::unit_test::test_case const&) ()
   from /usr/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.58.0
[#10](/bitcoin-bitcoin/10/) 0x00007ffff70b609e in boost::unit_test::framework_impl::visit(boost::unit_test::test_case const&) ()
   from /usr/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.58.0
[#11](/bitcoin-bitcoin/11/) 0x00007ffff70ec4cb in boost::unit_test::traverse_test_tree(boost::unit_test::test_suite const&, boost::unit_test::test_tree_visitor&) ()
   from /usr/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.58.0
[#12](/bitcoin-bitcoin/12/) 0x00007ffff70ec4cb in boost::unit_test::traverse_test_tree(boost::unit_test::test_suite const&, boost::unit_test::test_tree_visitor&) ()
   from /usr/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.58.0
[#13](/bitcoin-bitcoin/13/) 0x00007ffff70b19f6 in boost::unit_test::framework::run(unsigned long, bool) ()
   from /usr/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.58.0
[#14](/bitcoin-bitcoin/14/) 0x00007ffff70cd287 in boost::unit_test::unit_test_main(bool (*)(), int, char**) ()
   from /usr/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.58.0
[#15](/bitcoin-bitcoin/15/) 0x0000555555751e04 in main (argc=1442473840, argv=0x555556201460) at /usr/include/boost/test/unit_test.hpp:59

Assembly code:

   0x0000555555a18275 <+517>:   jmpq   0x555555a181f0 <CWallet::ReorderTransactions()+384>
=> 0x0000555555a1827a <+522>:   ud2    
   0x0000555555a1827c <+524>:   mov    %rax,%r14
   0x0000555555a1827f <+527>:   mov    0x60(%rsp),%rdi
   0x0000555555a18284 <+532>:   cmp    %rbx,%rdi

Looks lke a jump "between instructions" or such. Likely a messedup function pointer or other memory corruption. Weird.

in src/wallet/wallet.cpp:None in 653f3610b7 outdated

 680 | +    std::vector<int64_t> nOrderPosOffsets;
 681 | +    for (TxItems::iterator it = txByTime.begin(); it != txByTime.end(); ++it)
 682 | +    {
 683 | +        CWalletTx *const pwtx = (*it).second.first;
 684 | +        CAccountingEntry *const pacentry = (*it).second.second;
 685 | +        int64_t& nOrderPos = (pwtx != 0) ? pwtx->nOrderPos : pacentry->nOrderPos;

paveljanik commented at 6:07 PM on September 30, 2016:

+wallet/wallet.cpp:684:14: warning: declaration shadows a field of 'CWallet' [-Wshadow]
+    int64_t& nOrderPosNext = nOrderPosNext;
+             ^
+./wallet/wallet.h:659:13: note: previous declaration is here
+    int64_t nOrderPosNext;
+            ^
+wallet/wallet.cpp:684:30: warning: reference 'nOrderPosNext' is not yet bound to a value when used within its own initialization [-Wuninitialized]
+    int64_t& nOrderPosNext = nOrderPosNext;
+             ~~~~~~~~~~~~~   ^~~~~~~~~~~~~
+2 warnings generated.

pstratem commented at 10:10 AM on October 30, 2016:

indeed that is the issue

in src/wallet/wallet.cpp:None in 653f3610b7 outdated

 673 | +    BOOST_FOREACH(CAccountingEntry& entry, acentries)
 674 | +    {
 675 | +        txByTime.insert(make_pair(entry.nTime, TxPair((CWalletTx*)0, &entry)));
 676 | +    }
 677 | +
 678 | +    int64_t& nOrderPosNext = nOrderPosNext;

paveljanik commented at 6:09 PM on September 30, 2016:

What is this?

laanwj commented at 6:17 PM on September 30, 2016:

Yes, I think this is the culprit. It's creating a strange circular reference.

With this line removed it passes the tests.

MarcoFalke commented at 3:28 PM on October 18, 2016:

@pstratem Are you still working on this?

Move CWalletDB::ReorderTransactions to CWallet 86029e72c9

pstratem force-pushed on Oct 30, 2016

laanwj merged this on Nov 2, 2016

laanwj closed this on Nov 2, 2016

laanwj referenced this in commit a4fd8dff68 on Nov 2, 2016

codablock referenced this in commit a8cfd6c404 on Sep 19, 2017

codablock referenced this in commit b720415189 on Jan 13, 2018

andvgal referenced this in commit 5f4fd4a736 on Jan 6, 2019

CryptoCentric referenced this in commit 6750b293c9 on Feb 15, 2019

zkbot referenced this in commit 5b194067ea on Aug 12, 2021

MarcoFalke locked this on Sep 8, 2021