[Bitcoin-development] Newly introduced DoS

[Bitcoin-development] Newly introduced DoS

[Luke-Jr]

+        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.

Re: [Bitcoin-development] Newly introduced DoS

[Gavin Andresen]

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).

-- 
--
Gavin Andresen

Re: [Bitcoin-development] Newly introduced DoS

[Luke-Jr]

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.

Re: [Bitcoin-development] Newly introduced DoS

[Gavin Andresen]

> 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...

-- 
--
Gavin Andresen

Re: [Bitcoin-development] Newly introduced DoS

[Luke-Jr]

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.

Re: [Bitcoin-development] Newly introduced DoS

[theymos]

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.

Re: [Bitcoin-development] Newly introduced DoS

[Gavin Andresen]

> 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.  :-)

-- 
--
Gavin Andresen

Re: [Bitcoin-development] Newly introduced DoS

[Luke-Jr]

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?

Re: [Bitcoin-development] Newly introduced DoS

[Gregory Maxwell]

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.

Re: [Bitcoin-development] Newly introduced DoS

[Gavin Andresen]

> @@ -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.


-- 
--
Gavin Andresen