[Fix] Fix sigops handling pre nov upgrade #51
Conversation
|
My node is syncing past the offending block with this patch |
|
It seems pretty hacky to have to pass the boolean around for just this one fork. If/when other forks change behaviors in these functions we'll need to add booleans for them as well. The I haven't had a chance yet to go in further and estimate the feasibility of doing that. Obviously the current active flag set is known somewhere because the |
| @@ -419,21 +419,21 @@ func CheckProofOfWork(block *bchutil.Block, powLimit *big.Int) error { | |||
| // input and output scripts in the provided transaction. This uses the | |||
| // quicker, but imprecise, signature operation counting mechanism from | |||
| // txscript. | |||
| func CountSigOps(tx *bchutil.Tx) int { | |||
| func CountSigOps(tx *bchutil.Tx, magneticAnomalyActive bool) int { | |||
| msgTx := tx.MsgTx() | |||
|
|
|||
| // Accumulate the number of signature operations in all transaction | |||
| // inputs. | |||
| totalSigOps := 0 | |||
| for _, txIn := range msgTx.TxIn { | |||
There was a problem hiding this comment.
Had a chat with Josh during the hackathon. I'm not sure if this is the right place, but sigops should not be counted for coinbase inputs.
There was a problem hiding this comment.
@jasonbcox it looks to me like abc is validating the coinbase input https://github.com/Bitcoin-ABC/bitcoin-abc/blob/efa9749bac3e20b2a8fc056c01d75e3dbc4e341d/src/validation.cpp#L1717
There was a problem hiding this comment.
I think the correct thing to do is only check the coinbase outputs. The input can be garbage. I think we hit an unlikely byte sequence and it randomly broke on this block. I want to try syncing with just coinbase input validation removed and see if that fixes the issue. Then we don't need to pass around booleans everywhere. The code for that would be MUCH cleaner if that worked.
https://bitcoin.stackexchange.com/questions/4990/what-is-the-format-of-coinbase-input-scripts
There was a problem hiding this comment.
Actually I take that back. We need the mining interface to be accurate, and if ABC is counting those sigops even when the coinbase input has garbage data then we need to be consistent. I can investigate the txScript.Engine suggestion as I agree passing a boolean is a bit dirty.
That being said, my node is now stuck, so if we merge this change set then it unblocks people to start syncing, and we can refactor it to use the flags approach which will be a much better solution long term.
What do you think @tyler-smith ?
I think that can happen when we need it. Looks like the coinbase input was the culprit and if we fix that then these changes won't be necessary yet. If we do need to adjust the counting logic later then we can investigate |
|
@zquestz That sounds alright. It seems like we need to get some clarity from ABC on how exactly they're intending to handle coinbase tx sigop counts. |
There was a problem hiding this comment.
This looks correct and is tested.
Issue to track refactoring of the boolean flag for the fork is here: #52
So I believe this should fix the sigops counts pre anomaly. I am not 100% sure this is right, and I had to pass around the flag a lot. Maybe I missed something.
Hopefully this is useful, if so we can write some tests. =)
Also if anyone wants to make changes, feel free to push to this branch.
Start of fix for #47