Use CHECKCOLDSTAKEVERIFY_LOF for coldstakingoutput

[PeterL73]

Issue being fixed: discord

Any clue why a cold staking node would get this? The input is unspent (and delegated to my Cold Staking node), it's attempted to stake many times and it always fails with this message.

2022-10-25T15:31:15Z ERROR: SignCoinStake : failed to sign coinstake
2022-10-25T15:31:15Z ERROR: Unable to sign coinstake with input <REDACTED>-1

analysis
vout[1] has OP_CHECKCOLDSTAKEVERIFY. OP_CHECKCOLDSTAKEVERIFY will be the standard when V6 is enforced, for now OP_CHECKCOLDSTAKEVERIFY_LOF is. When OP_CHECKCOLDSTAKEVERIFY is used all outputs must have the same pubKeyScript. Pre V6 the masternode payment is in the last output, it doesn't have the same pubKeyScript so the check fails.

What was done?

The OP_CODE used for coldstakingoutput was changed from OP_CHECKCOLDSTAKEVERIFY to OP_CHECKCOLDSTAKEVERIFY_LOF

How Has This Been Tested?

At GitHub:PIVX-Project/PIVX:/src/script/interpreter.cpp#L1388 was checked if CheckColdStake failed.
It did for coldstakeoutputs with OP_CHECKCOLDSTAKEVERIFY, it didn't for coldstakeoutputs with OP_CHECKCOLDSTAKEVERIFY_LOF.

Breaking Changes

TODO: Change back to CHECKCOLDSTAKEVERIFY on Consensus::UPGRADE_V6_0

Checklist:

• I have performed a self-review of my own code
• I have performed a test of my own code
• I have commented my code, particularly in hard-to-understand areas

[JSKitty]

This is interesting! 👀
What confuses me is that MPW has been using this for a very long time (since we released Cold Staking), and the Labs address has been staking for many months using the previous CCSV opcode/script without issue, but only in the past 2-3 weeks, this staking suddenly caused errors, for both Labs and other users.

I will run some delegations with your PR however and see if this hits anything (should stake within 2-3 days maximum); I suggest we make some small modifications so that the switchover post-v6 is automatic rather than manual (MPW is aware of block heights).

So we pass the height and a chainparams UPGRADE_V6_0 to bitTrx.js, then the cold staking input function can choose the correct opcode.

Thanks for the time with investigating, a bounty of course shall be attached to this PR for your research/debugging efforts. 💜

[JSKitty]

I've been trying to test this for a solid week on my PIVX v5.5.0rc cold pool, and unfortunately no luck: which is incredibly weird, since staking the same amount with Allnodes resulted in stakes every 2-4-ish days very consistently for many months.

I've switched the Labz pool back to PIVX v5.4.0, and will continue testing. 🙏

[PeterL73]

The PIVX version isn't the issue. The issue is the OP_CODE used or rather the name and value of the OP_CODE

Let me elaborate on that a little bit more

In PIVX-Project/PIVX#2275
PIVX changed the name from OP_CHECKCOLDSTAKEVERIFY to OP_CHECKCOLDSTAKEVERIFY_LOF
PIVX-Project/PIVX@b194386#diff-93c5ddf703879287802d651b68c9c6199644afe23db0dd3ff77e8d1ce3c20045R187
it's value remained 0xd1 (= 209)
And they introduced an OP_CODE with the name OP_CHECKCOLDSTAKEVERIFY and value 0xd2
PIVX-Project/PIVX@df11631#diff-93c5ddf703879287802d651b68c9c6199644afe23db0dd3ff77e8d1ce3c20045R188
This pull request got merged on 2021-05-11 and was first released in v5.3.0

On 2021-09-19 in 14701c8
The OP_CODE usage was refactored from value based to name based
14701c8#diff-c37ef34c80e966a932a437f8c65884db936a3f9b74bbf7ac60e034dda1ca3188R140-R142

const OP = {
⋮
// cold staking
  'CHECKCOLDSTAKEVERIFY_LOF': 0xd1, // last output free for masternode/budget payments
  'CHECKCOLDSTAKEVERIFY': 0xd2,
⋮

And the value for coldstaking outputs was changed from 209 (=0xd1)
14701c8#diff-8802cc49cf5a9eb9a9ec5ac1dad908afe4881078c7cc6c41e03a51978ba2fe02L128

buf.push(209);  // OP_CHECKCOLDSTAKEVERIFY

to the name OP['CHECKCOLDSTAKEVERIFY']
14701c8#diff-8802cc49cf5a9eb9a9ec5ac1dad908afe4881078c7cc6c41e03a51978ba2fe02R131

buf.push(OP['CHECKCOLDSTAKEVERIFY']);

As you can see, the coldstaking output now uses the value 0xd2 instead off 0xd1 as before
So when this commit got released on MyPIVXWallet, it started creating coldstaking outputs with an other OP_CODE value then intended

[Duddino]

We need to merge this, since the explorer doesn't recognize our current way of delegating. With Peter's changes everything works as expected

[Duddino]

tACK, I just solved merge conflicts

[panleone]

tACK: Awesome, we really needed this to fix cold staking, thanks Peter!

[JSKitty]

utACK

[JSKitty]

🪙 405 PIV bounty fulfilled and sent