* [Bitcoin-development] Newly introduced DoS
@ 2011-09-26 19:17 Luke-Jr
2011-09-26 20:47 ` Gavin Andresen
0 siblings, 1 reply; 10+ messages in thread
From: Luke-Jr @ 2011-09-26 19:17 UTC (permalink / raw)
To: bitcoin-development
+ 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.
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [Bitcoin-development] Newly introduced DoS
2011-09-26 19:17 [Bitcoin-development] Newly introduced DoS Luke-Jr
@ 2011-09-26 20:47 ` Gavin Andresen
2011-09-26 20:55 ` Luke-Jr
2011-09-27 20:08 ` Luke-Jr
0 siblings, 2 replies; 10+ messages in thread
From: Gavin Andresen @ 2011-09-26 20:47 UTC (permalink / raw)
To: Luke-Jr; +Cc: bitcoin-development
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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [Bitcoin-development] Newly introduced DoS
2011-09-26 20:47 ` Gavin Andresen
@ 2011-09-26 20:55 ` Luke-Jr
2011-09-26 21:38 ` Gavin Andresen
2011-09-27 20:08 ` Luke-Jr
1 sibling, 1 reply; 10+ messages in thread
From: Luke-Jr @ 2011-09-26 20:55 UTC (permalink / raw)
To: Gavin Andresen; +Cc: bitcoin-development
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.
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [Bitcoin-development] Newly introduced DoS
2011-09-26 20:55 ` Luke-Jr
@ 2011-09-26 21:38 ` Gavin Andresen
2011-09-26 21:53 ` Luke-Jr
0 siblings, 1 reply; 10+ messages in thread
From: Gavin Andresen @ 2011-09-26 21:38 UTC (permalink / raw)
To: Luke-Jr; +Cc: bitcoin-development
> 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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [Bitcoin-development] Newly introduced DoS
2011-09-26 21:38 ` Gavin Andresen
@ 2011-09-26 21:53 ` Luke-Jr
2011-09-26 22:34 ` theymos
2011-09-27 0:07 ` Gavin Andresen
0 siblings, 2 replies; 10+ messages in thread
From: Luke-Jr @ 2011-09-26 21:53 UTC (permalink / raw)
To: Gavin Andresen; +Cc: bitcoin-development
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.
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [Bitcoin-development] Newly introduced DoS
2011-09-26 21:53 ` Luke-Jr
@ 2011-09-26 22:34 ` theymos
2011-09-27 0:07 ` Gavin Andresen
1 sibling, 0 replies; 10+ messages in thread
From: theymos @ 2011-09-26 22:34 UTC (permalink / raw)
To: bitcoin-development
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Bitcoin-development] Newly introduced DoS
2011-09-26 21:53 ` Luke-Jr
2011-09-26 22:34 ` theymos
@ 2011-09-27 0:07 ` Gavin Andresen
1 sibling, 0 replies; 10+ messages in thread
From: Gavin Andresen @ 2011-09-27 0:07 UTC (permalink / raw)
To: Luke-Jr; +Cc: bitcoin-development
> 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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Bitcoin-development] Newly introduced DoS
2011-09-26 20:47 ` Gavin Andresen
2011-09-26 20:55 ` Luke-Jr
@ 2011-09-27 20:08 ` Luke-Jr
2011-09-27 20:23 ` Gregory Maxwell
2011-09-27 20:39 ` Gavin Andresen
1 sibling, 2 replies; 10+ messages in thread
From: Luke-Jr @ 2011-09-27 20:08 UTC (permalink / raw)
To: Gavin Andresen; +Cc: bitcoin-development
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?
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [Bitcoin-development] Newly introduced DoS
2011-09-27 20:08 ` Luke-Jr
@ 2011-09-27 20:23 ` Gregory Maxwell
2011-09-27 20:39 ` Gavin Andresen
1 sibling, 0 replies; 10+ messages in thread
From: Gregory Maxwell @ 2011-09-27 20:23 UTC (permalink / raw)
To: Luke-Jr; +Cc: bitcoin-development
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Bitcoin-development] Newly introduced DoS
2011-09-27 20:08 ` Luke-Jr
2011-09-27 20:23 ` Gregory Maxwell
@ 2011-09-27 20:39 ` Gavin Andresen
1 sibling, 0 replies; 10+ messages in thread
From: Gavin Andresen @ 2011-09-27 20:39 UTC (permalink / raw)
To: Luke-Jr; +Cc: bitcoin-development
> @@ -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
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2011-09-27 20:39 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-09-26 19:17 [Bitcoin-development] Newly introduced DoS Luke-Jr
2011-09-26 20:47 ` Gavin Andresen
2011-09-26 20:55 ` Luke-Jr
2011-09-26 21:38 ` Gavin Andresen
2011-09-26 21:53 ` Luke-Jr
2011-09-26 22:34 ` theymos
2011-09-27 0:07 ` Gavin Andresen
2011-09-27 20:08 ` Luke-Jr
2011-09-27 20:23 ` Gregory Maxwell
2011-09-27 20:39 ` Gavin Andresen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox