Fix for names and native segwit + tests #274
Conversation
|
As usual, I will merge this if there are no objections. Since this is a somewhat more risky change, though (even if I'm pretty sure that it is safe since segwit is not activated yet), I would really like someone else to take at least a brief look - @JeremyRand, if you have time, please just take a brief look to confirm that you agree this change is safe to merge. |
|
I think we should also backport this to the |
|
@JeremyRand: Any chance you could take a brief look at this in the next couple of days? |
@domob1812 I'll try to find time to look at this soon, sorry for the delay (things are a bit busy here and I haven't had much time for Namecoin Core lately). |
No worries, that's fine. Also please don't feel obliged to do a detailed review - but since this is potentially risky, I just want someone else to take a look and confirm that the change is safe for Namecoin (with segwit not activated yet). |
|
@JeremyRand (and others): I'll merge this next week if there are no objections until then. |
The new unit test file script_segwit_tests.cpp contains some basic unit tests for CScript::IsPayToWitnessScriptHash and CScript::IsWitnessProgram.
a0c57c7 to
e431f31
Compare
When the segwit-code was originally integrated with Namecoin, we did not update CScript::IsWitnessProgram to be aware of name prefixes. This has the effect that names sent to a pure segwit script (e.g. a bech32 address) can be updated by anyone even after segwit is activated for Namecoin. P2SH-Segwit addresses are working already as expected. In this change, we fix that bug. With the fix, IsWitnessProgram strips a name prefix if it exists before extracting the witness program, so that pure segwit scripts can safely hold not only coins but also names after segwit activates. Note that this is in theory a consensus change, but it can be safely committed to Namecoin as segwit is not yet activated and thus such scripts are considered anyone-can-spend with and without this change at the moment. This also includes new regression and unit tests for names and segwit, including (but not limited to) those suggested by Jeremy in namecoin#258.
e431f31 to
3c8e3a6
Compare
|
Rebased, will merge now. |
3c8e3a6 Strip name prefix for IsWitnessProgram. (Daniel Kraft) 729d587 Unit tests for IsWitnessProgram and IsP2WSH. (Daniel Kraft) Pull request description: This fixes an issue with names and native segwit scripts (bech32 addresses). `CScript::IsWitnessProgram` did not strip name prefixes, which meant that names at bech32 addresses were anyone-can-spend even after segwit activation - but the transaction had to be included in a block, since the transaction would be rejected by a non-mandatory rule from the mempool. P2SH-Segwit is already working as expected. With this fix, names should work well together with segwit in all variants. This also includes tests to verify that, including (but not limited to) the situations described in #258. Note that this is in theory a consensus change, but it is safe to commit because segwit is not yet active for Namecoin. This includes a cherry-pick from bitcoin/bitcoin#14752, to create a place for the new name/segwit unit tests. Tree-SHA512: 01ec39dfcb2b167d3393dfa403c924ba9b71052eb5c93ff817f71b11ecad1ff3f78fefe6d97fa5bf7b53b16a905008707662ca35e5f80d88e9056ffedf23af36
Includes the segwit fix from upstream Namecoin, namecoin/namecoin-core#274. This fix has already been cherry-picked onto 1.1 and is live on the network, now it is also part of the "master" branch.
This fixes an issue with names and native segwit scripts (bech32 addresses).
CScript::IsWitnessProgramdid not strip name prefixes, which meant that names at bech32 addresses were anyone-can-spend even after segwit activation - but the transaction had to be included in a block, since the transaction would be rejected by a non-mandatory rule from the mempool. P2SH-Segwit is already working as expected.With this fix, names should work well together with segwit in all variants. This also includes tests to verify that, including (but not limited to) the situations described in #258.
Note that this is in theory a consensus change, but it is safe to commit because segwit is not yet active for Namecoin.
This includes a cherry-pick from bitcoin/bitcoin#14752, to create a place for the new name/segwit unit tests.