Fix wrong wallet RPC context set after #21366 #21572
Conversation
This bug doesn't have any effects currently because it only affects external signer RPCs which aren't currently using the wallet context, but it does cause an appveyor failure in a upcoming PR: https://ci.appveyor.com/project/DrahtBot/bitcoin/builds/38512882 This bug is subtle and could have been avoided if JSONRPCRequest didn't have constructors that were so loose with type checking. Suggested change bitcoin#21366 (comment) eliminates these and would be a good followup for a future PR.
This just makes an additional simplification after bitcoin#21366 replaced util::Ref with std::any. It was originally suggested bitcoin#21366 (comment) but delayed for a followup. It would have prevented usage bug bitcoin#21572.
There was a problem hiding this comment.
Code-review ACK 937fd4a
This bug is subtle and could have been avoided if JSONRPCRequest didn't
have constructors that were so loose with type checking.
Though I fully support the follow-up PR (actually I also planned to open it with your suggested changes in #21366 (comment)) , I don't think the removal of the ctors alone prevents bugs like this. Nothing stops you from setting the
JSONRPCRequest member variable "context" to "m_context" instead of "&m_context" and you have exactly the same problem...
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
This just makes an additional simplification after bitcoin#21366 replaced util::Ref with std::any. It was originally suggested bitcoin#21366 (comment) but delayed for a followup. It would have prevented usage bug bitcoin#21572.
It's true the new code in #21574 doesn't stop you from writing Any case, thanks for reviewing this bugfix and feel free to post any concerns about #21574 in the thread there if there are problems with that cleanup or ways to improve it. |
This just makes an additional simplification after bitcoin#21366 replaced util::Ref with std::any. It was originally suggested bitcoin#21366 (comment) but delayed for a followup. It would have prevented usage bug bitcoin#21572.
This just makes an additional simplification after bitcoin#21366 replaced util::Ref with std::any. It was originally suggested bitcoin#21366 (comment) but delayed for a followup. It would have prevented usage bug bitcoin#21572.
…1366 937fd4a Fix wrong wallet RPC context set after bitcoin#21366 (Russell Yanofsky) Pull request description: This bug doesn't have any effects currently because it only affects external signer RPCs which aren't currently using the wallet context, but it does cause an appveyor failure in a upcoming PR: https://ci.appveyor.com/project/DrahtBot/bitcoin/builds/38512882 This bug is subtle and could have been avoided if JSONRPCRequest didn't have constructors that were so loose with type checking. Suggested change bitcoin#21366 (comment) eliminates these and would be a good followup for a future PR. This PR just implements the simplest possible fix. ACKs for top commit: theStack: Code-review ACK 937fd4a meshcollider: Code review ACK 937fd4a Tree-SHA512: 53e6265ed6c7abb47d2b3e77d1604edfeb993c3a2440f0c19679cfeb23516965e6707ff486196a0acfbeff21c79a9a08b5cd33bae9a232d33d0134bca1bd0ff3
9044522 Drop JSONRPCRequest constructors after #21366 (Russell Yanofsky) Pull request description: This just makes an additional simplification after #21366 replaced util::Ref with std::any. It was originally suggested bitcoin/bitcoin#21366 (comment) but delayed for a followup. It would have prevented usage bug bitcoin/bitcoin#21572. ACKs for top commit: promag: ACK 9044522, fixed conflict in src/wallet/interfaces.cpp. Tree-SHA512: e909411b8f75013620b94e1a609296befb832fdcb574cd2e6689bfe3c636b03cd4ac1ccb2b32b532daf0f2131bb043464024966310fffc7e3cad77713d4bd0ef
…1366 9044522 Drop JSONRPCRequest constructors after bitcoin#21366 (Russell Yanofsky) Pull request description: This just makes an additional simplification after bitcoin#21366 replaced util::Ref with std::any. It was originally suggested bitcoin#21366 (comment) but delayed for a followup. It would have prevented usage bug bitcoin#21572. ACKs for top commit: promag: ACK 9044522, fixed conflict in src/wallet/interfaces.cpp. Tree-SHA512: e909411b8f75013620b94e1a609296befb832fdcb574cd2e6689bfe3c636b03cd4ac1ccb2b32b532daf0f2131bb043464024966310fffc7e3cad77713d4bd0ef
This just makes an additional simplification after bitcoin#21366 replaced util::Ref with std::any. It was originally suggested bitcoin#21366 (comment) but delayed for a followup. It would have prevented usage bug bitcoin#21572.
This bug doesn't have any effects currently because it only affects
external signer RPCs which aren't currently using the wallet context,
but it does cause an appveyor failure in a upcoming PR:
https://ci.appveyor.com/project/DrahtBot/bitcoin/builds/38512882
This bug is subtle and could have been avoided if JSONRPCRequest didn't
have constructors that were so loose with type checking. Suggested
change
#21366 (comment)
eliminates these and would be a good followup for a future PR.
This PR just implements the simplest possible fix.