P2pool does not validate that shares make valid blocks

[jtoomim]

Currently, p2pool does not perform any full validation of blocks or transactions. A peer can send a share that corresponds to a block that is invalid because it exceeds blocksize or sigop limits, because it includes a transaction with an invalid signature, because it contains a double-spend, because it builds upon an invalid block, because it builds on an extremely old block, or any other issue. This allows a malicious node to collect revenue from p2pool without performing any useful work for p2pool.

Note: In #19 I am advocating for removing fast share/block propagation code from p2pool, which would make it more difficult to add full validation.

[jtoomim]

The following are a few comments that I'm moving from #19 in order to keep that issue focused on implementation details for that bounty.

[jtoomim]

Quoth @CryptoManiac:

It would be harder to implement than you may think. Because having full set of transactions is needed not only to broadcast blocks, which was not a point of this mechanism but merely a bonus.

The main purpose of maintaining a global transaction set is an ability to re-build block and its header from share and verify its integrity. Otherwise it would be possible to create shares, which are valid by the terms of p2pool protocol, though giving you invalid blocks after rebuilding. This is technically an equivalent to block withholding attack, since such miner will get a part of future reward while doing maliciously invalid work.

[jtoomim]

Quoth @jtoomim:

It is already possible to create shares which are valid by the terms of the p2pool protocol but which are invalid as blocks. The idea that p2pool currently validates transactions or blocks is mistaken. Block withholding attacks are entirely possible on p2pool. Doing this is as simple as telling p2pool to mine a transaction that spends a nonexistent UTXO, or including a double-spend.

Currently, there are two incentives which discourage block withholding attacks. First, the individual who mines a valid block gets 0.5% of the block reward as a bonus. Second, each valid block pays all pool contributors. Thus, a miner with 5% of p2pool's hashrate who mines invalid blocks is foregoing 5.5% of their potential revenue. We are currently ensuring that miners contribute valid work with economics, not validation.

That economic assurance is not particularly strong, and it would be nice if we improved upon it. We could increase that 0.5% bonus to 5.0%, for example. Even better, it would be nice if we didn't rely on this economic incentive to ensure that blocks created by contributors were valid. But it would also be nice if we didn't have a constant 10% DOA+orphan rate on BTC and the inability to mine BCH blocks bigger than about 2 MB. Between those two directions, I think the second is more important for now.

At some later time, it might be nice to add a system that asynchronously fully validated shares/blocks in the background, outside of the latency-critical path. Implementing this system does not require a list of transactions to be encoded in the p2pool-specific metadata of a share, as the transaction set can already be verified using the header's merkle root hash.

[jtoomim]

Quoth @rldleblanc:

I'm trying to understand the concerns being raised here, so please bear with me.

The concern does not come from a rouge miner because the stratum protocol only allows the miner to send back (worker_name, job_id, extraonce2, ntime, nonce) (maybe more if we add the stratum extensions), so the miner can't muck with the merkel root, coinbase etc. The rouge miner could muck with those things, but would have to submit it to a local bitcoind. So I don't see how a miner could inject a bad share into the share chain.

That leaves a misbehaving P2pool node who is submitting faked data for the merkel root, coinbase, etc for the share. It seems that coinbase could be validated easily by computing the payouts and using a variance threshold see if the coinbase matches, if it doesn't then mark the share as bad. Merkel could be very tricky because it could have aux pow which another node might not have, so it is not easy to verify. But is that really critical? Seems like coinbase is more important and the worst they could do is mine an empty block with a full coinbase. That would still take just as much hash power, right?

Please help me understand what I'm missing here.

[jtoomim]

Quoth @krzr1s:

Please help me understand what I'm missing here.

Hello!
Check please - rldleblanc/p2pool@27f4727#commitcomment-31689585

[jtoomim]

@rldleblanc The issue that @CryptoManiac raised is a misbehaving p2pool node, not misbehaving mining hardware. You are correct that this attack still requires just as much hashpower as mining honestly, and it generates less revenue than mining honestly. Consequently, I consider it to be a medium-severity issue.

That said, it is not entirely a hypothetical issue. It is a real issue that we struggled with on the LTC p2pool for a while, as a node with about 3% of p2pool's hashrate was consistently mining invalid LTC blocks due to (apparently) modified p2pool code that miscalculated the fees. You can read about this on the forum at this link and on the following page:

https://forum.bitcoin.com/pools/p2pool-decentralized-dos-resistant-trustless-censorship-resistant-pool-t69932.html#p147346

My preference for dealing with invalid blocks like this is to have p2pool do delayed propagation of the full block with all transactions, and asynchronously fully validate the block via a bitcoind RPC call that does not yet exist (e.g. verifyblockvalidityexceptpow), and only mine on a share for e.g. 30 seconds unless it has been validated. It will be helpful to have a fast full-block propagation mechanism in place for that, but critically, it will need to be an asynchronous method, which rules out the existing mechanism. Ideally, we would use bitcoind's own fast full-block propagation mechanisms for that, as trying to do any work in Python that's O(transaction_count) is something we want to avoid.

[rldleblanc]

Fast full-block propagation won't work for shares that do not meet the block difficulty, so those would have to be sent by P2pool, right? With a 32 MB block that could be expensive, especially if there are many shares before a block is found. I guess that is part of the reason for the tx hashes. It would be nice to say that share 2 contains all transactions of share 1 plus these new ones. Basically diff between shares. It seems to make sense to have this at least stubbed out in the new implementation. I just can't think of any way around passing the whole block around to verify. Like you said, taking it out of the main data path makes a lot of sense. And Bitcoind not having a way to validate that a block is valid without meeting the block difficulty puts the burden on us for now (something we can sub out as well).

[jtoomim]

Using bitcoind's fast block propagation code for p2pool shares would require modifying bitcoind's code slightly to allow propagation of weak blocks upon request. Doing that modification is probably easier than writing a performant system in python. Slightly changing the functionality of an existing system is usually easier than implementing a completely new system from scratch.

I just can't think of any way around passing the whole block around to verify.

I can: don't fully verify. That's what we've been doing on p2pool since 2012, and it's worked out okay so far. The losses that we've incurred as a result of not verifying have been about 0.1%, which is far less than a traditional pool's fee. Fixing the blocks-aren't-fully-validated issue is a would-be-nice feature, but it's not worth implementing if it will hamstring p2pool's performance, fairness, or reward variance.

[rldleblanc]

Hmm... Seems that if we require a success from Bitcoind submitblock before propagating it on the share chain, that would prevent bad blocks that meet block difficulty from getting in the share chain. If a P2pool node submits a share with block difficulty, but we never see the hash in the block chain, then we punish that node. The higher % their false block submissions, the lower their payouts. That may get a majority of the issues without all the work. We may have to attach a node id to the share, so that we can punish shares across all miners on a bad node, but if the same miner is on a good node at the same time those shares are counted okay. The only problem I see is that my nodes are so small that they hardly ever find blocks. So I could be mining bad for a long time and never know it.

[CryptoManiac]

@rldleblanc It doesn't matter if share makes block or not. Every one of them must be passed through full set of checkings like if it would be a valid block candidate. Every spent transaction output, signature, output value should be verified to satisfy the protocol conditions.

[CryptoManiac]

@jtoomim submitblock has the parameter which allows you to check the block proposal. This means running full set of protocol checkings except proof of work, i.e. it's exactly what do you need.

You need to ensure that block proposal capability is set while doing getblocktemplate requests, as well as to set a mutability flag for prevblock and some other fields.

[rldleblanc]

@CryptoManiac I'm not seeing the parameter that you are talking about in the bitcoin code:

src/rpc/mining.cpp

UniValue submitblock(const JSONRPCRequest& request)
{
    // We allow 2 arguments for compliance with BIP22. Argument 2 is ignored.
    if (request.fHelp || request.params.size() < 1 || request.params.size() > 2) {
        throw std::runtime_error(
            "submitblock \"hexdata\"  ( \"dummy\" )\n"
            "\nAttempts to submit new block to network.\n"
            "See https://en.bitcoin.it/wiki/BIP_0022 for full specification.\n"

            "\nArguments\n"
            "1. \"hexdata\"        (string, required) the hex-encoded block data to submit\n"
            "2. \"dummy\"          (optional) dummy value, for compatibility with BIP22. This value is ignored.\n"
            "\nResult:\n"
            "\nExamples:\n"
            + HelpExampleCli("submitblock", "\"mydata\"")
            + HelpExampleRpc("submitblock", "\"mydata\"")
        );
    }

Looking through the getblocktemplate code, it looks if mode is set to 'proposal', then you can add a 'data' section containing the block and it will be validated for you. This corroborates BIP-0023. It looks like it will only validate shares that are built on the current tip, so for a node coming up to speed it can't check old shares in the chain with this method. They should have been validated by other nodes already, but how can we verify that? I guess download the chain from multiple peers and pick the one that matches the most?

That just leaves the handling of transactions. Since each P2pool node should have all the transactions in a block, the share submitter only needs to send a list of txids or something that uniquely identifies them that is common across all nodes. The node can build the block and submit it to getblocktemplate. If any txids are not in the local node cache try again later in case bitcoind is behind, then give up after so much time or the next block arrives on the network and consider it dead/orphan.

If this sounds acceptable, then I'll see what I can do.

[CryptoManiac]

@rldleblanc https://github.com/bitcoin/bips/blob/master/bip-0023.mediawiki#Block_Proposal

Just enable proposals and you should be able to send block candidates via simple submitblock requests.

[jtoomim]

To reiterate, this issue is not a high priority, and I'm not offering a bounty for it right now. #19 is more important.

[rldleblanc]

Understood, but if I can design with the implementation in mind now, it will be less of a kluge to add it in later.