Use of undocumented/undefined boost::signals2 behavior in wallet

theuni commented at 7:52 pm on November 1, 2022: member

While working on a minimal rewrite for our remaining usage of boost::signals2 (POC branch here), I stumbled upon some undocumented/undefined behavior with our current usage.

Specifically the problem, introduced in #17261, is that signals aren’t intended to connect to other signals. Currently this is violated in the wallet: https://github.com/bitcoin/bitcoin/blob/master/src/wallet/wallet.cpp#L3518

See the boost discussion here: https://groups.google.com/g/boost-list/c/So4i8JXneJ0

On my WIP replacement branch I implemented it by chaining the calls as suggested (and as-intended in our code): https://github.com/theuni/bitcoin/blob/replace-boost-signals/src/btcsignals.h#L140 . This makes tests happy, so presumably that’s what’s going on with boost too, though there are details (like interim disconnections) which could give us trouble in the future.

Regardless of the fact that this works for now, I don’t think that we should be relying on it. I haven’t looked into the details, but I’m hoping it could be fixed with some simple stub functions.

Ping @achow101. Any easy/obvious workarounds?

theuni added the label Bug on Nov 1, 2022

MarcoFalke added the label Wallet on Nov 1, 2022

achow101 commented at 9:18 pm on November 1, 2022: member

CWallet itself does not actually need to have these signals itself, they really are just passthrough from the SPKMs. The exception is a few places where CWallet is doing something with a SPKM and needs to emit NotifyCanGetAddressesChanged, but it could just call the signal in the SPKM it’s working on in order to do that instead of having it’s own signal. So the obvious solution here would be to connect to the SPKM signals directly instead of CWallet’s, and remove the signals from CWallet entirely. This could be done with a helper (perhaps modifying ConnectScriptPubKeyManNotifiers?), although perhaps there might be issues with connecting the signal for SPKMs that are created afterwards.

furszy commented at 10:23 pm on November 1, 2022: member

although perhaps there might be issues with connecting the signal for SPKMs that are created afterwards.

The wallet could trigger a signal on every added SPKM. e.g. trigger NewScriptPubKeyMan(const uint256& id) so the GUI subscribes to it calling ConnectScriptPubKeyMan(const uint256& id, callback).

Still, have to say that I’m not so convinced about it. Independently from boost’s problem, I think that, layers division wise, the spkm signal should first pass through the wallet, which could perform some operations with it (update/refresh some state/cache), and then the wallet trigger the upper layers signal (or call a function in a signals dispatcher object like we do in the validation area).

Use of undocumented/undefined boost::signals2 behavior in wallet #26442