Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reject p2p message #3185

Merged
merged 5 commits into from
Nov 11, 2013
Merged

Reject p2p message #3185

merged 5 commits into from
Nov 11, 2013

Conversation

gavinandresen
Copy link
Contributor

Send "reject" messages to peers if their transactions or blocks are rejected.

See https://gist.github.com/gavinandresen/7079034 and the discussion on the bitcoin-development mailing list.

Tested by running three -regtest -debug=net -printtoconsole nodes, one of which was running with -fuzzmessagestest=11 to generate garbled blocks/transactions, and verified that reject messages were sent and reported correctly as new blocks were generated and transactions sent between them.

@petertodd
Copy link
Contributor

Can you explain what actions you expect implementations to take in response to a block rejection message?

@gavinandresen
Copy link
Contributor Author

@petertodd: once an implementation is mature, I expect it would do nothing in response to a block rejection message, because false-positives (attackers being annoying) will be more common than true positives.

While an implementation is being developed, I expect the developers will investigate every rejection message.

I thought I was pretty clear in the gist that is the purpose of this message:

"In short, giving peers feedback about why their blocks or transactions are dropped or why they are being banned should help interoperability between different implementations"

@mikehearn
Copy link
Contributor

Looks good to me. Thanks for doing this.

@jgarzik
Copy link
Contributor

jgarzik commented Nov 1, 2013

Looks easy to fill up the disk with a stream of remote 'reject' messages.

@mikehearn
Copy link
Contributor

If you're thinking of logs, they're rotated anyway, I thought (or could be). Anyway making log rotation work nicely is an unrelated change.

@jgarzik
Copy link
Contributor

jgarzik commented Nov 1, 2013

That's a dodge. There's no rate limiting, you're still streaming from remote -> local disk.

@petertodd
Copy link
Contributor

@gavinandresen Right, so you're basically saying block rejection messages could be useful for an alt-implementation that's being used for mining. We know that the state of computer science isn't close to the point where we can do that safely; I personally a half-dozen forking bugs across a few alt-implementations the other week after an hour or two of work spent auditing them. We don't want to encourage that, and giving them handy rejection messages does.

When it comes to blocks, the only useful feedback you can give them is "stop doing that". We don't need rejection messages for that; blocks get orphaned says that just fine already.

@petertodd
Copy link
Contributor

@jgarzik Looks to me like you could dump a 32MiB rejection message into your remotes logs, and what's worse is that I don't see anything stopping you from making fake logs by putting newlines in that rejection message. Ugh.

@mikehearn
Copy link
Contributor

@jgarzik It's not a dodge. You can fill up the logs just by connecting repeatedly as well, or sending messages that cause a Misbehaving(0) to be hit, or lots of other things. That's just not something that's currently in our threat model. If you want to strengthen the bitcoind threat model to include "attacker filling up logs" then go ahead and do that on a new set of changes. It's just irrelevant to this change. But I'd suggest working on the more problematic DoS attacks first.

Being able to make fake logs is perhaps more of an issue, but that can be resolved just by forbidding newline characters (or heck, spaces) in the messages.

@petertodd
Copy link
Contributor

@mikehearn @gavinandresen Should be forbidding anything non-ascii-standard, newline and carriage return, and limit messages to, say, 256 characters. (we did something similar with alert messages right?)

@gavinandresen
Copy link
Contributor Author

@jgarzik @petertodd : Reject messages are only logged if you are running -debug=net (or -debug which means "everything"). I am assuming that you will only run -debug=net when you are, you know, debugging, and probably controlling who you are connecting to.

I'll add a "print out only the first 111 bytes", that should be sufficient belt-and-suspenders.

@petertodd
Copy link
Contributor

@gavinandresen I run all my nodes with -debug=net all the time so that if something odd happens with the network (e.g. the chain fork) I can debug it; I've got stacks of disk space and io bandwidth. Filtering out garbage, especially newlines, is an absolute must.

Also, still NACK on block messages.

@gavinandresen
Copy link
Contributor Author

@petertodd: duly noted.

Everybody else: any objections? If pull tester is happy, I'm going to pull.

@gmaxwell
Copy link
Contributor

gmaxwell commented Nov 1, 2013

We probably shouldn't allow control characters in printfs from the network. I don't believe we do anywhere else (and I'd looked some back when the OSX unicode crasher happened). Beyond log entry emulation free access lets you trash peoples' terminals.

Running the text through a URL encode would probably be a trivial way to avoid trouble.

@gavinandresen
Copy link
Contributor Author

@gmaxwell : good point RE: -printtoconsole.

Two commits now-- the first refactors the code we have in alert for sanitizing a string into a util.h SanitizeString method. Please don't suggest super-optimizing it with a lookup table, this is not performance-critical code.

The second limits the text printed to debug.log if -debug or -debug=net to 111 safe characters.

@petertodd : I'd get less annoyed if you made helpful observations like "don't we do something like that processing alerts..." up front. Maybe it is just me, but I constantly get the feeling you are trying to torpedo other people's ideas instead of trying to work together to be helpful.

@petertodd
Copy link
Contributor

@gavinandresen I hadn't read the code until today.

Also, limit the string to 111 characters first, then sanitize it.

static const unsigned char REJECT_NONSTANDARD = 0x40;
static const unsigned char REJECT_DUST = 0x41;
static const unsigned char REJECT_INSUFFICIENTFEE = 0x42;
static const unsigned char REJECT_CHECKPOINT = 0x43;
Copy link
Member

Choose a reason for hiding this comment

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

I dislike having codes in the protocol that correspond to implementation-specific details (nonstandard, dust, insufficient fee, and checkpoint). I'd prefer just encoding the class of problem (p2p protocol violatiom, network rule violation, local client policy) in the code, and use the message to further describe as necessary. Otherwise we'll end up with many extensions over time, and obsolete codes.

Copy link
Member

Choose a reason for hiding this comment

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

It seems REJECT_OBSOLETE can both be used for an obsolete P2P protocol number, and an obsolete block version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, REJECT_OBSOLETE can be used for both. You get "reject" "block" OBSOLETE or "reject" "version" OBSOLETE.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

RE: encoding class and letting the string encode the detail:

Meh. Either we'll get obsolete codes or we'll have peers trying to parse strings to figure out if their transactions are rejected because they don't have enough fees or because they have a dusty output.

I vote for obsolete codes as the lesser of the two evils.

Copy link
Member

Choose a reason for hiding this comment

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

I would certainly not confuse a protocol problem with a consensus rule violation.

@gavinandresen
Copy link
Contributor Author

@jgarzik @sipa : I think I've addressed all your concerns, I'd really like to move on to bigger and better things (almost have a relay-first-doublespend pull ready), can I get "good enough to pull" ACKs ?

@gavinandresen
Copy link
Contributor Author

Rebased to fix conflicts with mempool refactor.

Fixed the alert unit tests so bumping protocol version doesn't make them break any more, and bumped the protocol version to 70002.

@laanwj
Copy link
Member

laanwj commented Nov 4, 2013

ACK

Implementation nit: in all call sites the reject reason message is kind of
duplicated in the logged error message, the the point of both simply being
reworded versions of each other. It may make sense to reuse the same
message except where two different messages are really needed (ie for
privacy/security concerns).

@mikehearn
Copy link
Contributor

Looks good to me

@Diapolo
Copy link

Diapolo commented Nov 4, 2013

@laanwj I thought the same, many strings seem just to be small modifications and tend to be dups even in some cases.

I regenerated the alert test data; now alerts are tested
against a protocol version way above the current protocol
version.

So we won't have to regenerate them every time we bump
PROTOCOL_VERSION in the future.
@gavinandresen
Copy link
Contributor Author

Rebased.

RE: duplicating strings for debug.log and the reject message: I decided a fix would be worth than the disease.

Can't resist: "A foolish consistency is the hobgoblin of little minds, adored by little statesmen and philosophers and divines." -- Ralph Waldo Emerson

@BitcoinPullTester
Copy link

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/69aada346f74c978b5d8be59a5d7c79be966ef8c for binaries and test log.
This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/
Contact BlueMatt on freenode if something looks broken.

gavinandresen added a commit that referenced this pull request Nov 11, 2013
@gavinandresen gavinandresen merged commit 16d5f2c into bitcoin:master Nov 11, 2013
@laanwj
Copy link
Member

laanwj commented Nov 11, 2013

@gavinandresen right, if it were translated messages it'd be different

@gavinandresen gavinandresen deleted the reject branch November 12, 2013 23:37
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants