Skip to content

Mitigate DoS and fix dcrd orphan error handling#517

Merged
jholdstock merged 4 commits into
decred:masterfrom
jholdstock:ticketdelay
Jun 22, 2026
Merged

Mitigate DoS and fix dcrd orphan error handling#517
jholdstock merged 4 commits into
decred:masterfrom
jholdstock:ticketdelay

Conversation

@jholdstock

Copy link
Copy Markdown
Member

First two commits look huge but they are mostly indentation changes.

Closes #509

A malicious client could send an unlimited number of requests which
deliberately enter the txBroadcast loop, stalling a request handler for
7 seconds. This was a potential DoS vector which could make the server
unavailable for legitimate users.

Gating this loop with a semaphore mitigates the risk. A low limit of 16
pending broadcasts is chosen because this loop should only be entered
rarely during legitimate vspd usage.
String matching for this error is horribly brittle, as evidenced by the
fact that it was already broken for years and not noticed, however there
is no other way to do this (afaik).

@davecgh davecgh left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 on reversing the error checks. I know it's mostly just a style thing, but I very much prefer the fail fast style over burying the core logic and errors in layers of indentation.

Not only does it read better (to me anyway), but I also find it easier to reason about. "Bad stuff is out of the way, now to the positive path" versus "some good stuff here, but, oh, if something bad in that path, well, oh, but first the good stuff in that path, and then, ...". It is similar to why a lot of people like exceptions.

e.g:

if bad {
	/* return error */
}
if bad2 {
	/* return error2 */
}
/* do the core logic */

over

if good {
	/* do some core logic */
	if good2 {
		/* do some more core logic */
	} else {
		/* return error2 */
	}
} else {
	/* return error */
}

While computers are fast enough that it doesn't really make a big difference, especially when it's not in the hot path, not only is the former style much easier to read (imo), it is also almost always slightly faster due to the way the branch predictor works in modern processors.

Also, thanks for splitting out the wrapper and logic change into separate commits to make it easier to review.

Comment thread rpc/dcrd.go

// ErrOrphan error string is defined in dcrd/internal/mempool. Copied here
// because it is not exported.
var ErrOrphan = regexp.MustCompile(`orphan transaction \w+ references output \w+:\d+ of unknown or fully-spent transaction`)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
var ErrOrphan = regexp.MustCompile(`orphan transaction \w+ references output \w+:\d+ of unknown or fully-spent transaction`)
var ErrOrphan = regexp.MustCompile(`orphan transaction [[:xdigit:]]+ references output [[:xdigit:]]+:\d+ of unknown or fully-spent transaction`)

It doesn't really matter, but that is technically more accurate because they'll always be hashes in hex.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, I didn't know regex in go supported that kind of bracketed token/placeholder (I'm also not sure of the proper term to describe \w, \d etc 😆).

I'm going to leave it as-is for now because I've already validated this by running it on https://testnet-vsp.jholdstock.uk for a week, but will remember it for next time.

@jholdstock jholdstock merged commit 7e6d8d7 into decred:master Jun 22, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Potential DoS in middleware (currently unreachable)

2 participants