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

feat: Add specific error type for -rpcuser and -rpcpassword usage #72

Conversation

oleonardolima
Copy link
Contributor

@oleonardolima oleonardolima commented Jul 27, 2022

Description

As discussed in #68 trying to use the -rpcuser and -rpcpassword leads to an infinite loop, because this authentication method does not work alongside cookie authentication leading to an empty file, and the BitcoinD keeps retrying indefinitely.

This PR adds:

  • a new Error variant for this RpcUserAndPasswordUse
  • a new function to validate the args specified
  • two new tests, one for -rpcuser and -rpcpassword usage, which asserts a failure, and another one for the usage of -rpcauth which works fine alongside cookie authentication.

Fixes #68

- add a new fn to validate args
- return an Error if any specified arg is invalid
- invalid arguments are defined in a const regex string
- update and reuse previous tests for the bug
@RCasatta
Copy link
Collaborator

Concept ACK.

I would prefer not to add the "regex" crate if not strictly necessary, in this case, I think fn starts_with could do the job without an additional dependency?

Without regex this PR is an improvement and already mergeable to me.

A further improvement is to make args field a new type Args struct with a couple of functions to add an arg (at this point check arg validity) and to returns the args. This struct should consider also #39

@oleonardolima oleonardolima force-pushed the feat/add-error-type-for-invalid-args branch from 0fe2d62 to 15bff6a Compare July 28, 2022 18:53
- also fix cosmetics warning from ci
- also fix failing tests for versions <= 0_20_1
@oleonardolima oleonardolima force-pushed the feat/add-error-type-for-invalid-args branch from 15bff6a to 54e444a Compare July 28, 2022 19:17
@oleonardolima
Copy link
Contributor Author

Concept ACK.

I would prefer not to add the "regex" crate if not strictly necessary, in this case, I think fn starts_with could do the job without an additional dependency?

Without regex this PR is an improvement and already mergeable to me.

A further improvement is to make args field a new type Args struct with a couple of functions to add an arg (at this point check arg validity) and to returns the args. This struct should consider also #39

@RCasatta Thanks for the review and the suggestion

Yep, it does not really need the regex to do it, I updated it to have an invalid argument list and check from that list using the starts_with instead.

@RCasatta
Copy link
Collaborator

utACK 54e444a

Ideally when commits in the same PR touch the same lines touched by other commits in the same PR is better to squash, next time :)

@RCasatta RCasatta merged commit f0b12aa into rust-bitcoin:master Jul 29, 2022
@oleonardolima
Copy link
Contributor Author

oleonardolima commented Jul 29, 2022

utACK 54e444a

Ideally when commits in the same PR touch the same lines touched by other commits in the same PR is better to squash, next time :)

Thanks, I forgot to squash it, sorry 😁

I'll try to take a look at the other improvement to use Args

@oleonardolima oleonardolima deleted the feat/add-error-type-for-invalid-args branch January 22, 2023 16:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot use -rpcuser and -rpcpassword as arguments, does not return Err and enters infinite loop
2 participants