Correctly integrate CNoteData::witnessHeight into wallet code #1858

Merged
merged 4 commits into from Nov 15, 2016

Conversation

Projects
None yet
4 participants
@str4d
Contributor

str4d commented Nov 15, 2016

Closes #1715.

str4d added some commits Nov 15, 2016

@bitcartel

This comment has been minimized.

Show comment
Hide comment
@bitcartel

bitcartel Nov 15, 2016

Contributor
  • Maybe constant instead of magic number -1.
  • Perhaps move assert(nWitnessCacheSize > 0); to be under nWitnessCacheSize -= 1; so that it appears before loop to // Check the validity of the cache
  • Implement this? // TODO: If nWitnessCache is zero, we need to regenerate the caches (#1302)
Contributor

bitcartel commented Nov 15, 2016

  • Maybe constant instead of magic number -1.
  • Perhaps move assert(nWitnessCacheSize > 0); to be under nWitnessCacheSize -= 1; so that it appears before loop to // Check the validity of the cache
  • Implement this? // TODO: If nWitnessCache is zero, we need to regenerate the caches (#1302)
@str4d

This comment has been minimized.

Show comment
Hide comment
@str4d

str4d Nov 15, 2016

Contributor
  • The -1 is taken from the implicit "no chain" height in CChain::Height(), which by definition doesn't have a defined constant. Perhaps it would be better to just use < 0 instead, although had I done so before, the assertion wouldn't have triggered and we wouldn't have identified this bug (although technically, maybe it wouldn't have been a problem then - but the other bugs we found along the way definitely are).
  • It's somewhat cosmetic (as is the loop that got added in between for sanity checking), so I'd be inclined to leave it as-is for the purpose of making the diff easier to read. We can move it when implementing #1302.
  • No, that should be in a separate PR (at the very least, it would not be implementable for 1.0.3).
Contributor

str4d commented Nov 15, 2016

  • The -1 is taken from the implicit "no chain" height in CChain::Height(), which by definition doesn't have a defined constant. Perhaps it would be better to just use < 0 instead, although had I done so before, the assertion wouldn't have triggered and we wouldn't have identified this bug (although technically, maybe it wouldn't have been a problem then - but the other bugs we found along the way definitely are).
  • It's somewhat cosmetic (as is the loop that got added in between for sanity checking), so I'd be inclined to leave it as-is for the purpose of making the diff easier to read. We can move it when implementing #1302.
  • No, that should be in a separate PR (at the very least, it would not be implementable for 1.0.3).
@ebfull

This comment has been minimized.

Show comment
Hide comment
@ebfull

ebfull Nov 15, 2016

Contributor

ACK

@zkbot r+

Contributor

ebfull commented Nov 15, 2016

ACK

@zkbot r+

@zkbot

This comment has been minimized.

Show comment
Hide comment
@zkbot

zkbot Nov 15, 2016

Contributor

📌 Commit a4ef3aa has been approved by ebfull

Contributor

zkbot commented Nov 15, 2016

📌 Commit a4ef3aa has been approved by ebfull

zkbot pushed a commit that referenced this pull request Nov 15, 2016

zkbot
Auto merge of #1858 - str4d:1715-wallet-assertion, r=ebfull
Correctly integrate CNoteData::witnessHeight into wallet code

Closes #1715.
@zkbot

This comment has been minimized.

Show comment
Hide comment
@zkbot

zkbot Nov 15, 2016

Contributor

⌛️ Testing commit a4ef3aa with merge 4e4ca6d...

Contributor

zkbot commented Nov 15, 2016

⌛️ Testing commit a4ef3aa with merge 4e4ca6d...

@zkbot

This comment has been minimized.

Show comment
Hide comment
@zkbot

zkbot Nov 15, 2016

Contributor

☀️ Test successful - zcash

Contributor

zkbot commented Nov 15, 2016

☀️ Test successful - zcash

@zkbot zkbot merged commit a4ef3aa into zcash:master Nov 15, 2016

1 check passed

homu Test successful
Details
@bitcartel

This comment has been minimized.

Show comment
Hide comment
@bitcartel

bitcartel Nov 15, 2016

Contributor

@ebfull I didn't ACK this yet, still reviewing and looking at tickets #1715 and #1794. @str4d I'll continue to add any comments to this ticket and if we need to make any changes for release we can open a new PR.

Contributor

bitcartel commented Nov 15, 2016

@ebfull I didn't ACK this yet, still reviewing and looking at tickets #1715 and #1794. @str4d I'll continue to add any comments to this ticket and if we need to make any changes for release we can open a new PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment