Regression introduced in #17621 which causes p2sh-segwit addresses to be erroneously missed.
Tests are only failing in 0.19 branch, likely because that release still uses p2sh-segwit addresses rather than bech32 by default.
I’ll devise a test case to catch this going forward.
Tests are only failing in 0.19 branch, likely because that release still uses p2sh-segwit addresses rather than bech32 by default.
All wallet tests should probably be run on all possible address types. See also #17199 (review)
pushed a very non-comprehensive commit that would have stopped me in my tracks if I thought “nesting” worked with ScriptHash destination.
I can keep poking at something more comprehensive, but I felt this was unobtrusive enough to be a non-controversial start.
86 {
87 ScriptHash() : uint160() {}
88+ // These don't do what you'd expect.
89+ // Use ScriptHash(GetScriptForDestination(...)) instead.
90+ explicit ScriptHash(const WitnessV0KeyHash& hash) = delete;
91+ explicit ScriptHash(const WitnessV0ScriptHash& hash) = delete;
GetScriptForDestination in the constructor?
Yes, but that means I have to think more if we want to support more conversions, like ScriptHash(PKHash()) etc.
Like, is this a general tool for generating output scripts, or what?
Instead of deleting them, couldn’t you just call GetScriptForDestination in the constructor?
Sounds like begging for sharp edges in backporting…
PKHash instead since that is possible
Code review ACK 6dd59d2e491bc11ab26498668543e65440a3a931
I like the fact this reduces the chance of a similar future mistake. It certainly isn’t obvious at a glance.
84 struct ScriptHash : public uint160
85 {
86 ScriptHash() : uint160() {}
87+ // These don't do what you'd expect.
88+ // Use ScriptHash(GetScriptForDestination(...)) instead.
89+ explicit ScriptHash(const WitnessV0KeyHash& hash) = delete;
explicit here?
CKeyID also be deleted?
Maybe we should just make the inheritance private?
In that case, why not go all the way, and use composition instead of inheritance.
But let’s do this fix for 0.19 then maybe do something more comprehensive for 0.20.
