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

rpc: require second argument only for scantxoutset start action #17728

Merged
merged 1 commit into from Dec 15, 2019

Conversation

@achow101
Copy link
Member

achow101 commented Dec 11, 2019

It was reported on IRC that scantxoutset's API was broken in 0.19.0:

<belcher> i think scantxoutset may have been broken in bitcoin core 0.19 ? regardless of what parameters i run it with (e.g. "scantxoutset abort", "scantxoutset status") it just returns the help doc, according to the release notes the only change was https://github.com/bitcoin/bitcoin/pull/16285/files but i dont see anything that wouldve broken it, it works fine in 0.18
<belcher> im on regtest, in case its important
<harding> I can confirm `scantxoutset abort` returns the help doc on latest master.  Waiting for 0.18.1 to start now to attempt to reproduce there.
<harding> It looks like it's expecting a second parameter (even though that doesn't make sense with "abort").
<jonatack> Same for me as well
<harding> Can also confirm that `scantxoutset abort` returns the expected result on 0.18.1.

As noted in the conversation, previously, the second argument of scanobjects is only required for the start action. Stop and abort actions did not and could work without them.

It appears that this was broken by #16240 which enforced the size of the arguments to match the listed required arguments.

To fix this issue, this PR makes the scanobjects argument an optional argument. Then only in the start action do we check whether the scanobjects argument is there and throw an informative error about that. Also a test is added for this case.

The second argument of scanobjects is only required for the start action.
Stop and abort actions do not need this.
@promag

This comment has been minimized.

Copy link
Member

promag commented Dec 12, 2019

Approach ACK.

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Dec 12, 2019

ACK, this was introduced by me in fa0ad4e

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Dec 12, 2019

Probably because the previous documentation incorrectly labeled this as "required". See https://bitcoincore.org/en/doc/0.17.0/rpc/blockchain/scantxoutset/

Copy link
Member

promag left a comment

ACK 7d26357.

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Dec 15, 2019

ACK 7d26357
Thank you for fixing a clearly defined bug with a clear, minimal change.

laanwj added a commit that referenced this pull request Dec 15, 2019
…t action

7d26357 rpc: require second argument only for scantxoutset start action (Andrew Chow)

Pull request description:

  It was reported on [IRC](http://www.erisian.com.au/bitcoin-core-dev/log-2019-12-11.html#l-377) that `scantxoutset`'s API was broken in 0.19.0:

  ```
  <belcher> i think scantxoutset may have been broken in bitcoin core 0.19 ? regardless of what parameters i run it with (e.g. "scantxoutset abort", "scantxoutset status") it just returns the help doc, according to the release notes the only change was https://github.com/bitcoin/bitcoin/pull/16285/files but i dont see anything that wouldve broken it, it works fine in 0.18
  <belcher> im on regtest, in case its important
  <harding> I can confirm `scantxoutset abort` returns the help doc on latest master.  Waiting for 0.18.1 to start now to attempt to reproduce there.
  <harding> It looks like it's expecting a second parameter (even though that doesn't make sense with "abort").
  <jonatack> Same for me as well
  <harding> Can also confirm that `scantxoutset abort` returns the expected result on 0.18.1.
  ```

  As noted in the conversation, previously, the second argument of `scanobjects` is only required for the `start` action. `Stop` and `abort` actions did not and could work without them.

  It appears that this was broken by #16240 which enforced the size of the arguments to match the listed required arguments.

  To fix this issue, this PR makes the `scanobjects` argument an optional argument. Then only in the `start` action do we check whether the `scanobjects` argument is there and throw an informative error about that. Also a test is added for this case.

ACKs for top commit:
  laanwj:
    ACK 7d26357
  promag:
    ACK 7d26357.

Tree-SHA512: 828bdfe47f4fffa5d00a2cf88db6cea4a2714d9c49276841ca5cbdd1603b87bb6862147b86edcf36d7b40314ddb80b1a07fd399faf288572c55cc788c5cf9526
@laanwj laanwj merged commit 7d26357 into bitcoin:master Dec 15, 2019
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
sidhujag added a commit to syscoin/syscoin that referenced this pull request Dec 16, 2019
…et start action

7d26357 rpc: require second argument only for scantxoutset start action (Andrew Chow)

Pull request description:

  It was reported on [IRC](http://www.erisian.com.au/bitcoin-core-dev/log-2019-12-11.html#l-377) that `scantxoutset`'s API was broken in 0.19.0:

  ```
  <belcher> i think scantxoutset may have been broken in bitcoin core 0.19 ? regardless of what parameters i run it with (e.g. "scantxoutset abort", "scantxoutset status") it just returns the help doc, according to the release notes the only change was https://github.com/bitcoin/bitcoin/pull/16285/files but i dont see anything that wouldve broken it, it works fine in 0.18
  <belcher> im on regtest, in case its important
  <harding> I can confirm `scantxoutset abort` returns the help doc on latest master.  Waiting for 0.18.1 to start now to attempt to reproduce there.
  <harding> It looks like it's expecting a second parameter (even though that doesn't make sense with "abort").
  <jonatack> Same for me as well
  <harding> Can also confirm that `scantxoutset abort` returns the expected result on 0.18.1.
  ```

  As noted in the conversation, previously, the second argument of `scanobjects` is only required for the `start` action. `Stop` and `abort` actions did not and could work without them.

  It appears that this was broken by bitcoin#16240 which enforced the size of the arguments to match the listed required arguments.

  To fix this issue, this PR makes the `scanobjects` argument an optional argument. Then only in the `start` action do we check whether the `scanobjects` argument is there and throw an informative error about that. Also a test is added for this case.

ACKs for top commit:
  laanwj:
    ACK 7d26357
  promag:
    ACK 7d26357.

Tree-SHA512: 828bdfe47f4fffa5d00a2cf88db6cea4a2714d9c49276841ca5cbdd1603b87bb6862147b86edcf36d7b40314ddb80b1a07fd399faf288572c55cc788c5cf9526
fanquake added a commit to fanquake/bitcoin that referenced this pull request Jan 3, 2020
The second argument of scanobjects is only required for the start action.
Stop and abort actions do not need this.

Github-Pull: bitcoin#17728
Rebased-From: 7d26357
@fanquake fanquake mentioned this pull request Jan 3, 2020
luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Jan 5, 2020
The second argument of scanobjects is only required for the start action.
Stop and abort actions do not need this.

Github-Pull: bitcoin#17728
Rebased-From: 7d26357
@fanquake

This comment has been minimized.

Copy link
Member

fanquake commented Jan 6, 2020

Being backported in 17858.

laanwj added a commit that referenced this pull request Jan 8, 2020
99b5407 scripts: fix check-symbols & check-security argument passing (fanquake)
4330a1e Update msvc build for Visual Studio 2019 v16.4 (Aaron Clauson)
b0f9b8e Moves vcpkg list to a text file and updates the appveyor job and readme to use it. (Aaron Clauson)
cd7b3b2 Updated appveyor config:  - Update build image from Visual Studio 2017 to Visual Studio 2019.  - Updated Qt static library from Qt5.9.7 to Qt5.9.8.  - Added commands to update vcpkg port files (this does not update already installed packages).  - Updated vcpkg package list as per #17309.  - Removed commands setting common project file options. Now done via common.init.vcxproj include.  - Changed msbuild verbosity from normal to quiet. Normal rights a LOT of logs and impacts appveyor job duration. Updated msvc project configs:  - Updated platform toolset from v141 to v142.  - Updated Qt static library from Qt5.9.7 to Qt5.9.8.  - Added ignore for linker warning building bitcoin-qt program.  - Added missing util/str.cpp class file to test_bitcoin project file. (Aaron Clauson)
112144d Add missing typeinfo includes (Wladimir J. van der Laan)
1a6a534 net: Log to net category for exceptions in ProcessMessages (Wladimir J. van der Laan)
c0dc728 test: fix bitcoind already running warnings on macOS (fanquake)
5276b0e util: Add missing headers to util/fees.cpp (Hennadii Stepanov)
4d7875c rpc: require second argument only for scantxoutset start action (Andrew Chow)
bda2f5b cli: fix Fatal LevelDB error when specifying -blockfilterindex=basic twice (Harris)
d14ab7c gui: disable File->CreateWallet during startup (fanquake)
b9f1bc0 wallet: unbreak with boost 1.72 (Jan Beich)

Pull request description:

  Backports the following PRs to the 0.19 branch:
  * #17654 - Unbreak build with Boost 1.72.0
  * #17695 - gui: disable File->CreateWallet during startup
  * #17687 - cli: fix Fatal LevelDB error when specifying -blockfilterindex=basic twice
  * #17728 - rpc: require second argument only for scantxoutset start action
  * #17450 - util: Add missing headers to util/fees.cpp
  * #17488 - test: fix "bitcoind already running" warnings on macOS
  * #17762 - Log to net category for exceptions in ProcessMessages
  * #17364 - Updates to appveyor config for VS2019 and Qt5.9.8 + msvc project fixes
  * #17416 - Appveyor improvement - text file for vcpkg package list
  * #17736 - Update msvc build for Visual Studio 2019 v16.4
  * #17857 - scripts: fix symbol-check & security-check argument passing

  Fixes #17856.

ACKs for top commit:
  sipsorcery:
    ACK (tested: Windows 10 & msvc build) 99b5407.

Tree-SHA512: 91313de56fb0825e70a4be30ba0bf561b8c26d7dcf60549185df4f5e3524099398c828bb46faae807b631634d1afd5a1d397fb41e61ecfa0d746e4bf10b923cb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.