Fix wrong wallet RPC context set after #21366

ryanofsky commented at 5:00 pm on April 2, 2021: member

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.

Fix wrong wallet RPC context set after #21366

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
https://github.com/bitcoin/bitcoin/pull/21366#issuecomment-792044351
eliminates these and would be a good followup for a future PR.

937fd4a66f

ryanofsky referenced this in commit 2b98711426 on Apr 2, 2021

MarcoFalke requested review from theStack on Apr 2, 2021

DrahtBot added the label Wallet on Apr 2, 2021

theStack approved

theStack commented at 7:26 pm on April 2, 2021: member

Code-review ACK 937fd4a66f048780bffc5e714d0c800de987ce93

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…

DrahtBot commented at 1:30 am on April 3, 2021: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#21576 (rpc: bumpfee signer support by Sjors)
#21574 (Drop JSONRPCRequest constructors after #21366 by ryanofsky)
#21467 (Move external signer out of wallet module by Sjors)
#19101 (refactor: remove ::vpwallets and related global variables by ryanofsky)

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.

ryanofsky referenced this in commit 2dd05d5912 on Apr 3, 2021

ryanofsky commented at 3:50 am on April 6, 2021: member

Nothing stops you from setting the JSONRPCRequest member variable “context” to “m_context” instead of “&m_context” and you have exactly the same problem…

It’s true the new code in #21574 doesn’t stop you from writing request.context = whatever where whatever could be any type. But that’s just how std::any works. The reason I think #21574 would prevent this type of bug is just that it switches to clearly assigning an rpc request context, instead of passing an context argument to an intermediate function which could be accepting any type or value. Also the change would have required an update to the #ifdef ENABLE_EXTERNAL_SIGNER code block that was overlooked in the original PR because the old code still compiled even after it was no longer correct.

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.

meshcollider commented at 5:01 am on April 6, 2021: contributor

Code review ACK 937fd4a66f048780bffc5e714d0c800de987ce93

Thanks for the fix!

MarcoFalke merged this on Apr 7, 2021

MarcoFalke closed this on Apr 7, 2021

ryanofsky referenced this in commit 9044522ef7 on Apr 7, 2021

ryanofsky referenced this in commit 641ef117b3 on Apr 7, 2021

sidhujag referenced this in commit dd30e4b655 on Apr 7, 2021

MarcoFalke referenced this in commit 6664211be2 on Apr 8, 2021

sidhujag referenced this in commit 3d28753d27 on Apr 8, 2021

ryanofsky referenced this in commit b8c188e59e on Apr 8, 2021

DrahtBot locked this on Aug 18, 2022

Fix wrong wallet RPC context set after #21366 #21572

Conflicts