Newly introduced DoS

+ return DoS(10, error("AcceptToMemoryPool() : transaction with out-of- bounds SigOpCount")); + return DoS(10, error("ConnectInputs() : tried to spend coinbase at depth %d", pindexBlock->nHeight - pindex->nHeight)); + return DoS(10, error("AcceptBlock() : prev block not found"));

These shouldn't be "DoS"'d, or else you open a new DoS when nodes legitimately relay such transactions/blocks.

On Mon, Sep 26, 2011 at 3:17 PM, Luke-Jr <luke@dashjr•org> wrote:

> +        return DoS(10, error("AcceptToMemoryPool() : transaction with out-of-
> bounds SigOpCount"));
> +                        return DoS(10, error("ConnectInputs() : tried to
> spend coinbase at depth %d", pindexBlock->nHeight - pindex->nHeight));
> These shouldn't be "DoS"'d, or else you open a new DoS when nodes legitimately
> relay such transactions/blocks.

Huh?

So in the future lets suppose we schedule a change to the acceptable block rules that allows more SigOps in a block, or allows generation transaction to be spent before 100 confirmations. At that same time, the DoS rules will be changed.

You cannot "legitimately" relay those blocks without a scheduled block-chain-split. If a block-chain-split IS scheduled and the rules change, then denying service to nodes running old, obsolete versions of bitcoin is the right thing to do-- it is better to "fail hard" and find it difficult or impossible to connect to the network rather than continue with an obsolete client and a non-majority block chain.

(and the third DoS in AcceptBlock(): prev block not found is a "should be impossible" case, because AcceptBlock is only called when extending the best-block chain).

On Monday, September 26, 2011 4:47:06 PM Gavin Andresen wrote:

> On Mon, Sep 26, 2011 at 3:17 PM, Luke-Jr <luke@dashjr•org> wrote:
> > +        return DoS(10, error("AcceptToMemoryPool() : transaction with
> > out-of- bounds SigOpCount"));
> > +                        return DoS(10, error("ConnectInputs() : tried to
> > spend coinbase at depth %d", pindexBlock->nHeight - pindex->nHeight));
> > These shouldn't be "DoS"'d, or else you open a new DoS when nodes
> > legitimately relay such transactions/blocks.
>
> Huh?
>
> So in the future lets suppose we schedule a change to the acceptable
> block rules that allows more SigOps in a block, or allows generation
> transaction to be spent before 100 confirmations. At that same time,
> the DoS rules will be changed.
>
> You cannot "legitimately" relay those blocks without a scheduled
> block-chain-split.  If a block-chain-split IS scheduled and the rules
> change, then denying service to nodes running old, obsolete versions
> of bitcoin is the right thing to do-- it is better to "fail hard" and
> find it difficult or impossible to connect to the network rather than
> continue with an obsolete client and a non-majority block chain.
>
> (and the third DoS in AcceptBlock(): prev block not found  is a
> "should be impossible" case, because AcceptBlock is only called when
> extending the best-block chain).

The first one I was referring to is a *transaction* with "non-standard" sig op count, which is AFAIK allowed in blocks, just not accepted by the mainline rules. In the second case, that transaction is not tied to a specific block. Maybe the person spending it sees it matured beyond 100 confirmations, and you only see 99. An attacker could use these things to get nodes to ban each other.

> The first one I was referring to is a *transaction* with "non-standard" sig op
> count, which is AFAIK allowed in blocks, just not accepted by the mainline
> rules.

I sit corrected. The context is:
    // Checking ECDSA signatures is a CPU bottleneck, so to avoid
denial-of-service
    // attacks disallow transactions with more than one SigOp per 34
bytes.
    // 34 bytes because a TxOut is:
    //   20-byte address + 8 byte bitcoin amount + 5 bytes of ops + 1
byte script length
    if (GetSigOpCount() > nSize / 34 || nSize < 100)
	return DoS(10, error("AcceptToMemoryPool() : transaction with
out-of-bounds SigOpCount"));

I'm having trouble imagining some future world where valid, new-versions-agree-to-relay-transactions have more than one SigOp per 34 bytes; can you give an example?

> Maybe the person spending it sees it matured beyond 100 confirmations, and you
> only see 99. An attacker could use these things to get nodes to ban each
> other.

That would imply you're on a blockchain fork of more than 99 blocks with respect to the person spending the transaction, in which case I'd argue you have much bigger problems and it is a good idea for the DoS code to kick in and kick either you or them off the network...

On Monday, September 26, 2011 5:38:41 PM Gavin Andresen wrote:

> > The first one I was referring to is a *transaction* with "non-standard"
> > sig op count, which is AFAIK allowed in blocks, just not accepted by the
> > mainline rules.
>
> I sit corrected. The context is:
>     // Checking ECDSA signatures is a CPU bottleneck, so to avoid
> denial-of-service
>     // attacks disallow transactions with more than one SigOp per 34
> bytes.
>     // 34 bytes because a TxOut is:
>     //   20-byte address + 8 byte bitcoin amount + 5 bytes of ops + 1
> byte script length
>     if (GetSigOpCount() > nSize / 34 || nSize < 100)
> 	return DoS(10, error("AcceptToMemoryPool() : transaction with
> out-of-bounds SigOpCount"));
>
> I'm having trouble imagining some future world where valid,
> new-versions-agree-to-relay-transactions have more than one SigOp per
> 34 bytes; can you give an example?

It's not future. It's presently allowed in blocks. Which means it's perfectly valid to relay (and also perfectly value to NOT relay or accept). Ergo, shouldn't be punished.

> > Maybe the person spending it sees it matured beyond 100 confirmations,
> > and you only see 99. An attacker could use these things to get nodes to
> > ban each other.
>
> That would imply you're on a blockchain fork of more than 99 blocks
> with respect to the person spending the transaction, in which case I'd
> argue you have much bigger problems and it is a good idea for the DoS
> code to kick in and kick either you or them off the network...

Um, no? It implies you have 99 blocks since the coinbase, and he has 100 and wants to spend. In this scenario, it's proper to reject his transaction *until you have the next block*, but it doesn't make sense to punish for it.

On Monday, September 26, 2011 5:53 PM, "Luke-Jr" <luke@dashjr•org> wrote:

> It's not future. It's presently allowed in blocks. Which means it's
> perfectly valid to relay (and also perfectly value to NOT relay or
> accept). Ergo, shouldn't be punished.

Yeah, my node has always relayed these transactions. The limit seems pointless to me, especially when it's per kB: people will just add more data.

The coinbase maturity DoS limit should not have a chance of immediately kicking the node, as I believe this could happen normally in rare cases. Rejecting these transactions is also pretty cheap, AFAIK. A small DoS score seems reasonable, though.

> It's not future. It's presently allowed in blocks. Which means it's perfectly
> valid to relay (and also perfectly value to NOT relay or accept). Ergo,
> shouldn't be punished.

You're absolutely right.

And you're right about the 99 confirmations, too-- I was thinking blocks again, not transactions.

Good to get all of the wrong-ness out of my system on a Monday so I know I'll be perfect the rest of the week. :-)

What about this one?

@@ -1276,13 +1278,13 @@ bool CBlock::AcceptBlock()
     // Get prev block index
     map<uint256, CBlockIndex*>::iterator mi =
mapBlockIndex.find(hashPrevBlock);
     if (mi == mapBlockIndex.end())
-        return error("AcceptBlock() : prev block not found");
+        return DoS(10, error("AcceptBlock() : prev block not found"));

Is it certain that it cannot be triggered by a peer having some huge number more blocks than you?

On Tue, Sep 27, 2011 at 4:08 PM, Luke-Jr <luke@dashjr•org> wrote:

> Is it certain that it cannot be triggered by a peer having some huge number
> more blocks than you?

Might be better to have a global flag that indicates when the node thinks its current with the network (this could have other UI impacts, like letting the user know if they send and their connectivity looks non-current), and only enforce this check when the node believes that its current.

Currency could be =height>last_checkpoint&&top_timestamp>now()-safe_amount; with safe_amount to be high enough that it's very unlikely to be falsely triggered by an improbably long gap.

> @@ -1276,13 +1278,13 @@ bool CBlock::AcceptBlock()
>     // Get prev block index
>     map<uint256, CBlockIndex*>::iterator mi =
> mapBlockIndex.find(hashPrevBlock);
>     if (mi == mapBlockIndex.end())
> -        return error("AcceptBlock() : prev block not found");
> +        return DoS(10, error("AcceptBlock() : prev block not found"));
>
>
> Is it certain that it cannot be triggered by a peer having some huge number
> more blocks than you?

As I said, that is a "can't never happen but we'll wear a belt-and-suspenders just in case" case.

AcceptBlock() is called from two places in the code:

ProcessBlock, if the block is not an orphan:
    // If don't already have its previous block, shunt it off to
holding area until we get it
    if (!mapBlockIndex.count(pblock->hashPrevBlock))
    {
....  orphan processing stuff...
      return true;
    }

    // Store to disk
    if (!pblock->AcceptBlock())
        return error("ProcessBlock() : AcceptBlock FAILED");

The mapBlockIndex.find(hashPrevBlock) in AcceptBlock can't fail.

The second place is recursively, in AcceptBlock(), processing orphans that link to the block being accepted, and mapBlockIndex.find() would find the used-to-be-an-orphan-block-that-is-now-being-accepted.

So: it is a case that should be impossible to trigger. However, in case there is some subtle bug or edge case I'm not considering it seem to me keeping the check is appropriate, and, because it will be a subtle bug or edge case, it seems to me keeping the DoS penalty is also appropriate, because attackers look for subtle bugs and edge cases that can be exploited.