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: 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”
@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.
@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.
@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.
@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.
@petertodd: duly noted.
Everybody else: any objections? If pull tester is happy, I’m going to pull.
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.
@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.
@gavinandresen I hadn’t read the code until today.
Also, limit the string to 111 characters first, then sanitize it.
69+static const unsigned char REJECT_OBSOLETE = 0x11;
70+static const unsigned char REJECT_DUPLICATE = 0x12;
71+static const unsigned char REJECT_NONSTANDARD = 0x40;
72+static const unsigned char REJECT_DUST = 0x41;
73+static const unsigned char REJECT_INSUFFICIENTFEE = 0x42;
74+static const unsigned char REJECT_CHECKPOINT = 0x43;
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.
3771@@ -3723,8 +3772,12 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv)
3772 mapAlreadyAskedFor.erase(inv);
3773 int nDoS = 0;
3774 if (state.IsInvalid(nDoS))
3775+ {
3776+ pfrom->PushMessage("reject", strCommand, state.GetRejectCode(),
3777+ state.GetRejectReason(), inv.hash);
3970@@ -3918,6 +3971,29 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv)
3971 }
3972
3973
3974+ else if (strCommand == "reject")
Happy to write a BIP after we’re sure we’re happy with how it is working.
PROTOCOL_VERSION bump: disagree:
“Sending “reject” messages to old nodes does no harm– unknown commands are ignored for extensibility (in the reference implementation, at least– other implementations should do the same). So there is no need to bump the protocol version.”
… but don’t really care and will sway to consensus.
@mikehearn Sounds pretty reasonable to me.
I vote for a bump too - use 70002 and I’ll update the NODE_BLOOM patch to 70003.
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.
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).
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.
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
