Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Move CWalletDB::ReorderTransactions to CWallet #8828

Merged
merged 1 commit into from Nov 2, 2016
Merged

Move CWalletDB::ReorderTransactions to CWallet #8828

merged 1 commit into from Nov 2, 2016

Conversation

pstratem
Copy link
Contributor

@pstratem pstratem commented Sep 28, 2016 •
edited

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
@@ -155,6 +155,7 @@ class CWalletDB : public CDB

/// This writes directly to the database, and will not update the CWallet's cached accounting entries!
/// Use wallet.AddAccountingEntry instead, to write *and* update its caches.
bool WriteAccountingEntry(const uint64_t nAccEntryNum, const CAccountingEntry& acentry);
Copy link
Contributor

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

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

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
{
CWalletTx *const pwtx = (*it).second.first;
CAccountingEntry *const pacentry = (*it).second.second;
int64_t& nOrderPos = (pwtx != 0) ? pwtx->nOrderPos : pacentry->nOrderPos;
Copy link
Contributor

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

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
txByTime.insert(make_pair(entry.nTime, TxPair((CWalletTx*)0, &entry)));
}

int64_t& nOrderPosNext = nOrderPosNext;
Copy link
Contributor

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

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

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

@maflcko maflcko maflcko left review comments

@paveljanik paveljanik paveljanik left review comments

Assignees
No one assigned
Labels
Wallet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@pstratem @paveljanik @laanwj @maflcko