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.

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

Fix calculating amount to migrate #3990

Merged
merged 3 commits into from May 8, 2019

Conversation

8 participants
@Eirik0
Copy link
Contributor

commented May 7, 2019

No description provided.

@mms710 mms710 added this to Needs Prioritization in Arborist Team via automation May 7, 2019

@mms710 mms710 added this to the v2.0.5 milestone May 7, 2019

@mms710 mms710 moved this from Needs Prioritization to Bugs/Security Issues/TechDebt Backlog in Arborist Team May 7, 2019

@mms710 mms710 moved this from Bugs/Security Issues/TechDebt Backlog to Current Sprint in Arborist Team May 7, 2019

@mms710 mms710 moved this from Current Sprint to In Review in Arborist Team May 7, 2019

@LarryRuane
Copy link
Contributor

left a comment

ACK, somehow my wallet got into a state in which I could reproduce this reliably; on this branch, the problem no longer occurs.

           Block height | 528536
            Connections | 8
  Network solution rate | 4048318169 Sol/s

You are currently not mining.
To enable mining, add 'gen=1' to your zcash.conf and restart.

Since starting this node 4 minutes, 55 seconds ago:
- You have validated 2098 transactions!

[Press Ctrl+C to exit] [Set 'showmetrics=0' to hide]

Thread 7 "zcashd" received signal SIGSEGV, Segmentation fault.
[Switching to LWP 4347]
0x0000555555c295fc in TransactionBuilder::CreateJSDescriptions (this=0x7fffdad3c700) at transaction_builder.cpp:531
531	                } else if (!coinsView->GetSproutAnchorAt(prevJoinSplit.anchor, tree)) {
(gdb) bt
#0  0x0000555555c295fc in TransactionBuilder::CreateJSDescriptions (this=0x7fffdad3c700) at transaction_builder.cpp:531
#1  0x0000555555c2815d in TransactionBuilder::Build (this=0x7fffdad3c700) at transaction_builder.cpp:349
#2  0x0000555555b5efe7 in AsyncRPCOperation_saplingmigration::main_impl (this=0x5555930c9560)
    at wallet/asyncrpcoperation_saplingmigration.cpp:126
#3  0x0000555555b5df40 in AsyncRPCOperation_saplingmigration::main (this=0x5555930c9560)
    at wallet/asyncrpcoperation_saplingmigration.cpp:31
#4  0x00005555559771c5 in AsyncRPCQueue::run (this=0x555556ebfec0, workerId=1) at asyncrpcqueue.cpp:68
#5  0x000055555597a5f1 in std::__invoke_impl<void, void (AsyncRPCQueue::*)(unsigned long), AsyncRPCQueue*, unsigned long> (__f=
    @0x555556ebeec8: (void (AsyncRPCQueue::*)(AsyncRPCQueue * const, unsigned long)) 0x555555976f72 <AsyncRPCQueue::run(unsigned long)>, __t=@0x555556ebeec0: 0x555556ebfec0, __args#0=@0x555556ebeeb8: 1)
    at /usr/include/c++/7/bits/invoke.h:73
#6  0x00005555559792d1 in std::__invoke<void (AsyncRPCQueue::*)(unsigned long), AsyncRPCQueue*, unsigned long> (__fn=
    @0x555556ebeec8: (void (AsyncRPCQueue::*)(AsyncRPCQueue * const, unsigned long)) 0x555555976f72 <AsyncRPCQueue::run(unsigned long)>, __args#0=@0x555556ebeec0: 0x555556ebfec0, __args#1=@0x555556ebeeb8: 1)
    at /usr/include/c++/7/bits/invoke.h:95
#7  0x000055555597f35d in std::thread::_Invoker<std::tuple<void (AsyncRPCQueue::*)(unsigned long), AsyncRPCQueue*, unsigned long> >::_M_invoke<0ul, 1ul, 2ul> (this=0x555556ebeeb8) at /usr/include/c++/7/thread:234
#8  0x000055555597f24e in std::thread::_Invoker<std::tuple<void (AsyncRPCQueue::*)(unsigned long), AsyncRPCQueue*, unsigned long> >::operator() (this=0x555556ebeeb8) at /usr/include/c++/7/thread:243
#9  0x000055555597f185 in std::thread::_State_impl<std::thread::_Invoker<std::tuple<void (AsyncRPCQueue::*)(unsigned long), AsyncRPCQueue*, unsigned long> > >::_M_run (this=0x555556ebeeb0) at /usr/include/c++/7/thread:186
#10 0x00007ffff7b0957f in ?? ()
#11 0x0000000000000000 in ?? ()
(gdb) p coinsView 
$2 = (const CCoinsViewCache *) 0x0
(gdb) 
@str4d

str4d approved these changes May 7, 2019

@str4d

This comment has been minimized.

Copy link
Contributor

commented May 7, 2019

@zkbot r+

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented May 7, 2019

📌 Commit c6d36dc has been approved by str4d

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented May 7, 2019

⌛️ Testing commit c6d36dc with merge b7c693b...

zkbot added a commit that referenced this pull request May 7, 2019

@mdr0id

This comment has been minimized.

Copy link
Contributor

commented May 7, 2019

FWIW, ACK

@mms710 mms710 moved this from In Review to Merge Queue in Arborist Team May 7, 2019

@bitcartel
Copy link
Contributor

left a comment

utACK (after review and discussion)

@bitcartel

This comment has been minimized.

Copy link
Contributor

commented May 7, 2019

We should open a ticket to add a test for the code path through: https://github.com/zcash/zcash/pull/3990/files#diff-c8b1f15b762bd763ce827e6172b4f296R101

assert(coinsView);
// We do not check cs_coinView becaue we do not set this in testing

This comment has been minimized.

Copy link
@bitcartel

bitcartel May 7, 2019

Contributor

Spelling: because

// assert that coinsView is not null
assert(coinsView);
// We do not check cs_coinView becaue we do not set this in testing
// assert(cs_coinsView);

This comment has been minimized.

Copy link
@bitcartel

bitcartel May 7, 2019

Contributor

It seems we want to assert here, but because the tests break, we are not going to. Which puts us back in the pre-PR position of not having an assertion, which contributed to the default. I suggest filing a ticket to resolve this post-release unless the tests can be updated to pass with the assertion in place.

This comment has been minimized.

Copy link
@Eirik0

Eirik0 May 7, 2019

Author Contributor

I agree that we do want to be asserting this. I will file a ticket to fix this post release.

@Eirik0

This comment has been minimized.

Copy link
Contributor Author

commented May 8, 2019

Force pushed with the spelling correction.

@bitcartel

This comment has been minimized.

Copy link
Contributor

commented May 8, 2019

ACK modulo the last two commits related to coinsView being squashed into one. The PR will then have three distinct commits.

@Eirik0 Eirik0 force-pushed the Eirik0:fix-migration-amount branch from ba9dcc6 to ea8823c May 8, 2019

@Eirik0

This comment has been minimized.

Copy link
Contributor Author

commented May 8, 2019

Squashed. Thanks @bitcartel

@Eirik0

This comment has been minimized.

Copy link
Contributor Author

commented May 8, 2019

@zkbot r+

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented May 8, 2019

📌 Commit ea8823c has been approved by Eirik0

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented May 8, 2019

⌛️ Testing commit ea8823c with merge 4fb4999...

zkbot added a commit that referenced this pull request May 8, 2019

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented May 8, 2019

💔 Test failed - pr-merge

@Eirik0

This comment has been minimized.

Copy link
Contributor Author

commented May 8, 2019

Looks like a timeout.

@Eirik0

This comment has been minimized.

Copy link
Contributor Author

commented May 8, 2019

@zkbot retry

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented May 8, 2019

⌛️ Testing commit ea8823c with merge 9270215...

zkbot added a commit that referenced this pull request May 8, 2019

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented May 8, 2019

☀️ Test successful - pr-merge
Approved by: Eirik0
Pushing 9270215 to master...

@zkbot zkbot merged commit ea8823c into zcash:master May 8, 2019

1 check passed

homu Test successful
Details

Arborist Team automation moved this from Merge Queue to Released (Merged in Master) May 8, 2019

@daira

This comment has been minimized.

Copy link
Contributor

commented May 8, 2019

Note that Larry's segfault isn't necessarily fixed by this PR (actually I don't see how this PR could fix it). It's more likely that it incidentally caused the segfault to no longer be reproducible.

Sorry, this PR does include a fix for the segfault.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.