net: deprecate p2p message reject #883
Conversation
Codecov Report
@@ Coverage Diff @@
## master #883 +/- ##
==========================================
- Coverage 61.67% 61.63% -0.04%
==========================================
Files 155 155
Lines 25976 25870 -106
==========================================
- Hits 16021 15946 -75
+ Misses 9955 9924 -31
Continue to review full report at Codecov.
|
|
One thing the reject packet does is resolve the corresponding |
|
Untested, LGTM. Code reads correct. |
Where does that happen? We wait for a packet from a network peer to resolve a job? |
| const reject = packets.RejectPacket.fromReason(code, reason, msg, hash); | ||
|
|
||
| if (msg) { | ||
| this.logger.debug('Rejecting %s %h (%s): code=%s reason=%s.', |
There was a problem hiding this comment.
I think it's important that we continue to log reject reasons, even if we don't send the packets.
There was a problem hiding this comment.
We may already be logging it internally, I'll check though.
There was a problem hiding this comment.
Perhaps increaseBan should log the reason, each time.
There was a problem hiding this comment.
I think it's important that we continue to log reject reasons, even if we don't send the packets.
I agree with this.
Perhaps increaseBan should log the reason, each time.
This seems like a smart place to log. I would like to know when my peers misbehave.
There was a problem hiding this comment.
Except now that I think about it, that could be a DOS vector regarding a disk fill attack when too many loglines
There was a problem hiding this comment.
Useful to consider, however it likely wouldn't be significant as at maximum it would be 100 lines, with increasing the score at 1 per time.
There was a problem hiding this comment.
To allow for logging the reason, we would need to change the function signature of peer.increaseBan. Right now it just takes a ban score to add, the second argument could be a msg.
this.logger.info('Increasing banscore for %s by %d: %s', this.hostname(), score, msg);What do you think?
There was a problem hiding this comment.
I am wondering about the DoS point, as there could be something there not considered.
There was a problem hiding this comment.
Maybe the logger should log on the debug level when logging about banscores. It would prevent possible attacks if we assume that production nodes should run with their logger on info
There was a problem hiding this comment.
Default logger level is debug:
Line 30 in f991b0a
|
In handleReject(peer) {
this.emit('reject', peer);
for (const job of this.jobs)
job.resolve(false);
this.jobs.length = 0;
}Resolve in this case is the |
This PR removes the
RejectPacketfrombcoin, following recent changes tobitcoind. I am not particularly opinionated on whether or not this gets merged, but am opening it up for discussion.One thing that needs more investigation are the internal codes for reject packets as there is no test coverage for them.
The
Poolwill now callpeer.increaseBaninstead of callingpeer.reject, which previously wrapperpeer.increaseBan.Part of the rationality of removing this packet was that it is not a recommended way to debug. If we do decide to remove the
RejectPacket, then we should also be sure that logging is sufficient.There is a case to be made about node diversity and supporting features that are not supported by
bitcoind, regarding @pinheadmz's suggestion to couple them withwhitebind.Closes #880