Skip to content

test: add coverage for invalid parameters for rescanblockchain #25906

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
Merged
maflcko merged 1 commit into from
Aug 24, 2022
Merged

test: add coverage for invalid parameters for rescanblockchain #25906

maflcko merged 1 commit into from
Aug 24, 2022

Conversation

@brunoerg
Copy link
Contributor

@brunoerg brunoerg commented Aug 22, 2022

This PR adds test coverage for the following errors:

bitcoin/src/wallet/rpc/transactions.cpp

Lines 880 to 894 in 2bd9aa5

if (!request.params[0].isNull()) {
start_height = request.params[0].getInt<int>();
if (start_height < 0 || start_height > tip_height) {
throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid start_height");
}
}
if (!request.params[1].isNull()) {
stop_height = request.params[1].getInt<int>();
if (*stop_height < 0 || *stop_height > tip_height) {
throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid stop_height");
} else if (*stop_height < start_height) {
throw JSONRPCError(RPC_INVALID_PARAMETER, "stop_height must be greater than start_height");
}
}

@fanquake fanquake added the Tests label Aug 22, 2022
aureleoules
aureleoules reviewed Aug 23, 2022
View reviewed changes
Copy link
Contributor

@aureleoules aureleoules left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 32cd1441d7e178b14ff63154585eaf0f38104cae - i verified this case was not tested.

Maybe outside the scope of this PR but this also seems to be untested.

bitcoin/src/wallet/rpc/transactions.cpp

Lines 896 to 899 in 2bd9aa5

// We can't rescan beyond non-pruned blocks, stop and throw an error
if (!pwallet->chain().hasBlocks(pwallet->GetLastBlockHash(), start_height, stop_height)) {
throw JSONRPCError(RPC_MISC_ERROR, "Can't rescan beyond pruned data. Use RPC call getblockchaininfo to determine your pruned height.");
}

w0xlt
w0xlt approved these changes Aug 23, 2022
View reviewed changes
Copy link
Contributor

@w0xlt w0xlt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 32cd144

@brunoerg
Copy link
Contributor Author

brunoerg commented Aug 23, 2022

@aureleoules

Maybe outside the scope of this PR but this also seems to be untested.

Cool, I can work on it for other PR. Thanks

kristapsk
kristapsk reviewed Aug 23, 2022
View reviewed changes
@brunoerg brunoerg force-pushed the 2022-08-add-test-rescanblockchain branch from 32cd144 to bd47bb5 Compare August 23, 2022 16:50
@brunoerg
Copy link
Contributor Author

brunoerg commented Aug 23, 2022

Force pushed addressing @kristapsk's review.

kristapsk
kristapsk approved these changes Aug 23, 2022
View reviewed changes
Copy link
Contributor

@kristapsk kristapsk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK bd47bb56681b0cc0b0c49e9b4e7f3d6e2ae5ad3e

w0xlt
w0xlt approved these changes Aug 23, 2022
View reviewed changes
Copy link
Contributor

@w0xlt w0xlt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reACK bd47bb5

But there is a lint error in CI.

@brunoerg brunoerg force-pushed the 2022-08-add-test-rescanblockchain branch from bd47bb5 to d1a0004 Compare August 23, 2022 20:14
@brunoerg
Copy link
Contributor Author

brunoerg commented Aug 23, 2022

But there is a lint error in CI.

Thanks, @w0xlt. It was fixed in last force-pushed.

w0xlt
w0xlt approved these changes Aug 24, 2022
View reviewed changes
Copy link
Contributor

@w0xlt w0xlt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reACK d1a0004

CI error in Win64 native [vs2022] check seems unrelated.

@maflcko maflcko merged commit 713ea7a into bitcoin:master Aug 24, 2022
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Aug 24, 2022
Merge bitcoin#25906: test: add coverage for invalid parameters for `r…
67425f4 
…escanblockchain`

d1a0004 test: add coverage for invalid parameters for `rescanblockchain` (brunoerg)

Pull request description:

  This PR adds test coverage for the following errors:
  https://github.com/bitcoin/bitcoin/blob/2bd9aa5a44b88c866c4d98f8a7bf7154049cba31/src/wallet/rpc/transactions.cpp#L880-L894

ACKs for top commit:
  w0xlt:
    reACK bitcoin@d1a0004

Tree-SHA512: c357fbda3d261e4d06a29d2a5350482db5f97a815adf59abdac1971eb19b69cfd4d54e4d21836851e2e3b116aa2a820ea1437c7aededf86b06df435cca16ac90
@bitcoin bitcoin locked and limited conversation to collaborators Aug 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

3 more reviewers

@aureleoules aureleoules aureleoules left review comments

@kristapsk kristapsk kristapsk approved these changes

@w0xlt w0xlt w0xlt approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@brunoerg @kristapsk @aureleoules @w0xlt @fanquake @maflcko