Skip to content

Move CWalletDB::ReorderTransactions to CWallet#8828

Merged
laanwj merged 1 commit intobitcoin:masterfrom
pstratem:2016-09-28-cwallet-reordertransactions
Nov 2, 2016
Merged

Move CWalletDB::ReorderTransactions to CWallet#8828
laanwj merged 1 commit intobitcoin:masterfrom
pstratem:2016-09-28-cwallet-reordertransactions

Conversation

@pstratem
Copy link
Contributor

@pstratem pstratem commented Sep 28, 2016 •
edited
Loading

CWalletDB was never the right place for this.

@maflcko maflcko added the Wallet label Sep 28, 2016
paveljanik
paveljanik reviewed Sep 30, 2016
View reviewed changes
src/wallet/walletdb.h Outdated
Copy link
Contributor

@paveljanik paveljanik Sep 30, 2016

Choose a reason for hiding this comment

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

Move this a line down because of the comment?

@paveljanik
Copy link
Contributor

paveljanik commented Sep 30, 2016

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
Copy link
Member

laanwj commented Sep 30, 2016

Concept ACK

@laanwj
Copy link
Member

laanwj commented Sep 30, 2016 •
edited
Loading

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  CWallet::ReorderTransactions (this=<optimized out>) at /.../bitcoin/bitcoin/src/wallet/wallet.cpp:681
#1  0x00005555557c5d19 in accounting_tests::GetResults (results=std::map with 0 elements)
    at /.../bitcoin/bitcoin/src/wallet/test/accounting_tests.cpp:24
#2  0x00005555557c1128 in accounting_tests::acc_orderupgrade::test_method (this=<optimized out>)
    at /.../bitcoin/bitcoin/src/wallet/test/accounting_tests.cpp:58
#3  0x00005555557c0cde in accounting_tests::acc_orderupgrade_invoker ()
    at /.../bitcoin/bitcoin/src/wallet/test/accounting_tests.cpp:32
#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  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  0x00007ffff70cecb1 in ?? () from /usr/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.58.0
#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  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  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 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 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 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 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 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 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.

paveljanik
paveljanik reviewed Sep 30, 2016
View reviewed changes
src/wallet/wallet.cpp Outdated
Copy link
Contributor

@paveljanik paveljanik Sep 30, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

+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.

Copy link
Contributor Author

@pstratem pstratem Oct 30, 2016

Choose a reason for hiding this comment

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

indeed that is the issue

paveljanik
paveljanik reviewed Sep 30, 2016
View reviewed changes
src/wallet/wallet.cpp Outdated
Copy link
Contributor

@paveljanik paveljanik Sep 30, 2016

Choose a reason for hiding this comment

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

What is this?

Copy link
Member

@laanwj laanwj Sep 30, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

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

With this line removed it passes the tests.

Copy link
Member

@maflcko maflcko Oct 18, 2016

Choose a reason for hiding this comment

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

@pstratem Are you still working on this?

@pstratem pstratem force-pushed the 2016-09-28-cwallet-reordertransactions branch from 653f361 to 86029e7 Compare October 30, 2016 09:16
@laanwj laanwj merged commit 86029e7 into bitcoin:master Nov 2, 2016
laanwj added a commit that referenced this pull request Nov 2, 2016
@laanwj
Merge #8828: Move CWalletDB::ReorderTransactions to CWallet
a4fd8df 
86029e7 Move CWalletDB::ReorderTransactions to CWallet (Patrick Strateman)
codablock pushed a commit to codablock/dash that referenced this pull request Jan 13, 2018
@laanwj @codablock
Merge bitcoin#8828: Move CWalletDB::ReorderTransactions to CWallet
b720415 
86029e7 Move CWalletDB::ReorderTransactions to CWallet (Patrick Strateman)
andvgal pushed a commit to energicryptocurrency/gen2-energi that referenced this pull request Jan 6, 2019
@laanwj @andvgal
Merge bitcoin#8828: Move CWalletDB::ReorderTransactions to CWallet
5f4fd4a 
86029e7 Move CWalletDB::ReorderTransactions to CWallet (Patrick Strateman)
CryptoCentric pushed a commit to absolute-community/absolute that referenced this pull request Feb 15, 2019
@laanwj @CryptoCentric
Merge bitcoin#8828: Move CWalletDB::ReorderTransactions to CWallet
6750b29 
86029e7 Move CWalletDB::ReorderTransactions to CWallet (Patrick Strateman)
@nuttycom nuttycom mentioned this pull request Aug 11, 2021
Merged
zkbot added a commit to zcash/zcash that referenced this pull request Aug 12, 2021
@zkbot
Auto merge of #5271 - nuttycom:backport/13825_kill_accounts, r=str4d
5b19406 
[Backport] bitcoin/bitcoin#13825 kill accounts

This is a backport of the following commits:

- from bitcoin/bitcoin#13825
- pick bitcoin/bitcoin@c9c32e6
  - from bitcoin/bitcoin#14023
    - pick bitcoin/bitcoin@f0dc850
      - from bitcoin/bitcoin#13566
        - pick bitcoin/bitcoin@702ae1e
        - pick bitcoin/bitcoin@cf15761
        - pick bitcoin/bitcoin@0f3d6e9
        - pick bitcoin/bitcoin@7110c83
        - pick bitcoin/bitcoin@ef7bc88
        - pick bitcoin/bitcoin@4279da4
    - pick bitcoin/bitcoin@c410f41
    - from bitcoin/bitcoin#9614
      - pick bitcoin/bitcoin@02d9f50
      - pick bitcoin/bitcoin@82b7dc3
        - from bitcoin/bitcoin#8061
          - pick bitcoin/bitcoin@ecb9741
  - from bitcoin/bitcoin#6851
    - pick bitcoin/bitcoin@3e7c89196c
  - from bitcoin/bitcoin#8828
    - pick bitcoin/bitcoin@86029e7
      - from bitcoin/bitcoin#8696
        - just the first two commits, as AccountingEntry is going away
        - pick bitcoin/bitcoin@d2e678d
        - pick bitcoin/bitcoin@59adc86
    - from bitcoin/bitcoin#8028
      - pick bitcoin/bitcoin@0fd5997
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@laanwj laanwj laanwj left review comments

+2 more reviewers

@maflcko maflcko maflcko left review comments

@paveljanik paveljanik paveljanik left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Wallet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@pstratem @paveljanik @laanwj @maflcko