Mitigate DoS and fix dcrd orphan error handling#517
Conversation
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
left a comment
There was a problem hiding this comment.
👍 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.
|
|
||
| // 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`) |
There was a problem hiding this comment.
| 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.
There was a problem hiding this comment.
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.
First two commits look huge but they are mostly indentation changes.
Closes #509