Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Various corrections to developer reference #1611

Merged
merged 4 commits into from May 26, 2017

Conversation

Projects
None yet
4 participants
Contributor

achow101 commented May 22, 2017

Corrected Compact Size Unsigned Integer description to include the lower bounds.

Changed reject message code descriptions of the extra data field to be more accurate as the extra data is not always what was specified nor is the field required.

Clarified the empty message headers are allowed.

Removed incorrect note in importaddress which stated that addresses would not be rescanned if already in the wallet. They are rescanned.

Closes #1505, #1365, #1604, #1520

If this is accepted, please pay the bounty to 1AjFwHanPewurVms4Z3ExtapHmKWF6aTVS

achow101 added some commits May 22, 2017

Make reject message code descriptions more accurate
Changed extra data *is* to *may include* since the extra data does not necessarily have to include what was stated.
Remove incorrect note in importaddress
Remove incorrect note that said imporaddress would not rescan stuff already in the wallet. Those addresses and scripts already in the wallet will still be rescanned.

@wbnns wbnns self-assigned this May 23, 2017

Contributor

harding commented May 23, 2017

@achow101 in and of themselves, these changes LGTM. I was wondering if you checked the implementation behavior to confirm all of those issues were accurate? (I'm not saying you have to, I don't know what @wbnns's bounty rules are, but I usually like to confirm the problems before editing the docs.)

In particularly, I'm surprised by #1520 (rescan) as I don't think I would've added the extra sentence there unless I believed it was true based on my reading of the code or my testing (though it's possible the implementation has changed since I wrote it, or that I was wrong when I wrote it, or that I'm wrong now in guessing at my mental state 2.5 years ago when I wrote it).

Also #1365 (compactSize canonical encoding) has a link to a Bitcoin Core dev saying that the sizes weren't previously enforced, suggesting to me that there may be some subtleties in the behavior or maybe that this was even an unknown soft fork.

Contributor

wbnns commented May 23, 2017

@achow101 Thanks for tackling this stuff! @harding Thanks also for the feedback.

Unless others object, this will be merged on Thursday, May 26th.

Contributor

achow101 commented May 23, 2017

@harding

I did verify the behavior and checked in the code.

For the rescan functionality, it looks like the behavior did change with this commit bitcoin/bitcoin@983d2d9 so it no longer did nothing if the address/script already existed.

For the CompactSize uint encoding, that behavior changed with this commit: bitcoin/bitcoin@8dc206a to enforce canonical encoding.

Contributor

harding commented May 23, 2017

@achow101 awesome! Thanks!

@wbnns wbnns merged commit bdbe5da into bitcoin-dot-org:master May 26, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment