namecoin / electrum-nmc

Namecoin port of Electrum Bitcoin client.
https://www.namecoin.org/
MIT License
29 stars 24 forks source link

Allow zero-output coinbase transactions in auxpow #230

Closed domob1812 closed 4 years ago

domob1812 commented 4 years ago

Even though a transaction without outputs is not valid, it can be deserialised fine by Namecoin Core and thus is accepted as parent coinbase in an auxpow. Electrum-NMC's deserialiser already fails on that, and thus Electrum-NMC did so far not accept those auxpow's (leading to a consensus failure with Namecoin Core).

With this patch, we allow auxpow's whose parent coinbase transaction has no outputs, so that Electrum-NMC follows the behaviour of Namecoin Core.

Note that auxpow's like that are very unlikely to occur on mainnet, as a miner would essentially have to build a Namecoin-only block for it, giving up the merge-mining bitcoin reward (and thus it is very costly to do so). But it is nevertheless a possibility. On testnet, there are actual blocks that failed to sync with Electrum-NMC before this patch (e.g. the one from the new unit test).

JeremyRand commented 4 years ago

Concept ACK.

Shouldn't this be submitted to the auxpow branch rather than master?

Does this issue affect the master-3.3.10 branch too? If so, is it easy to backport? (Transaction parsing has been completely rewritten since then.)

Nit: Git commit message says "deserialising (and verifying) and auxpow"; I assume "and" is a typo and should be "an".

domob1812 commented 4 years ago

Thanks for the review! I agree this should be for auxpow, and have changed it accordingly. I've also fixed the commit-message typo.

I think this does not affect the pre-rewrite code, as it used to work with Xaya before that. It seems very likely that the reason why it broke was the rewrite.

JeremyRand commented 4 years ago

Looks good to me except for the grammar typo I noted. Once that's fixed, I think this can be merged.

domob1812 commented 4 years ago

Thanks for the review, I've pushed a fix.