Drop the return and corruptionPossible arguments from CValidationState::DoS, and rename to ::Reject #12463

Empact commented at 8:48 PM on February 16, 2018: member

These arguments were always false. With this, DoS and Invalid return false always, as with error.

instagibbs commented at 9:03 PM on February 16, 2018: member

concept ACK, anything that gets rid of some of that nightmare argument list is a plus

Empact commented at 12:16 AM on February 17, 2018: member

Note other PRs in this area: #11523 #11639

fanquake added the label Refactoring on Feb 17, 2018

practicalswift commented at 4:29 PM on February 24, 2018: contributor

Nice cleanup!

Concept ACK

laanwj commented at 6:22 PM on March 7, 2018: member

Concept ACK

conscott commented at 4:25 AM on March 17, 2018: contributor

Test ACk 829f5461cd9ba850bededff78640ce1807ffcee2

MarcoFalke commented at 3:52 PM on April 2, 2018: member

Needs rebase due to "witness reserved value" rename.

jnewbery commented at 8:32 PM on April 2, 2018: member

utACK 829f5461cd9ba850bededff78640ce1807ffcee2

Empact force-pushed on Apr 2, 2018

Empact commented at 11:21 PM on April 2, 2018: member

Rebased & ran clang-format.

jnewbery commented at 4:19 PM on April 3, 2018: member

utACK 228e3f9965b66f23c611a944eed9454ca5d433eb

MarcoFalke commented at 5:44 PM on April 3, 2018: member

utACK 228e3f9

sipa commented at 10:54 PM on April 3, 2018: member

This is a bit scary, due to the large number of optional arguments to DoS there could be overlooked cases that still compile. I'd be more comfortable reviewing if it was clear that all call sites have been touched. You could do this by either writing it as a scripted diff, or by renaming the method DoS to something else.

MarcoFalke commented at 11:17 PM on April 3, 2018: member

Good catch. It seems it must be renamed, since a scripted diff could still lead to silent merge conflicts.

() Obviously could do it as a scripted-diff *and rename.

Empact commented at 12:22 AM on April 4, 2018: member

I did something like that here: #12357, basically, privatize DoS, remove unnecessary argument defaults, and introduce "Corrupt" to live alongside "Invalid" as the public interface, both calling into DoS. Does that sound reasonable? I'd need to update it.

Empact commented at 12:29 AM on April 4, 2018: member

I have a somewhat different take in mind, will push that up in a bit.

Empact force-pushed on Apr 4, 2018

Empact commented at 12:59 AM on April 4, 2018: member

* Introduced ::Corrupt to sit alongside ::Invalid * Dropped DoS in favor of the two above * Removed another random boolean argument in the process

~~So the only accidental compilable change would require using ::Invalid, where the method signature has changed by adding a leading required int.~~

jnewbery commented at 1:55 PM on April 4, 2018: member

Travis job timed out. I've restarted it.

in src/consensus/validation.h:40 in 7f64db16e1 outdated

  36 | @@ -37,24 +37,21 @@ class CValidationState {
  37 |      std::string strDebugMessage;
  38 |  public:
  39 |      CValidationState() : mode(MODE_VALID), nDoS(0), chRejectCode(0), corruptionPossible(false) {}
  40 | -    bool DoS(int level, bool ret = false,
  41 | -             unsigned int chRejectCodeIn=0, const std::string &strRejectReasonIn="",
  42 | -             bool corruptionIn=false,
  43 | -             const std::string &strDebugMessageIn="") {
  44 | +    bool Invalid(int level, unsigned int chRejectCodeIn, const std::string& strRejectReasonIn, const std::string& strDebugMessageIn = "")

jnewbery commented at 2:13 PM on April 4, 2018:

I think you can make strDebugMessageIn a required argument for this and Corrupt() below, since all calls to both functions specify a strDebugMessageIn. That removes the final optional argument.

Empact commented at 10:19 PM on April 4, 2018:

I'm seeing calls with just 3 args for each, e.g. validation.cpp, lines 561 and 566. strRejectReasonIn is consistently provided.

Empact commented at 10:32 PM on April 4, 2018:

It's interesting to consistently provide validation debug information though, that's a project I'm up for.

in src/consensus/validation.h:45 in 7f64db16e1 outdated

  45 | +    {
  46 |          chRejectCode = chRejectCodeIn;
  47 |          strRejectReason = strRejectReasonIn;
  48 | -        corruptionPossible = corruptionIn;
  49 |          strDebugMessage = strDebugMessageIn;
  50 |          if (mode == MODE_ERROR)

jnewbery commented at 2:14 PM on April 4, 2018:

Probably scope creep so feel free to ignore, but since you're changing things around so much, it'd be clearer to me to change this to:

if (mode != MODE_ERROR)

and unify the return falses at the end of the function.

MarcoFalke commented at 9:56 PM on April 4, 2018: member

See also discussion in #11523

Empact force-pushed on Apr 4, 2018

Empact force-pushed on Apr 5, 2018

Empact commented at 10:56 AM on April 5, 2018: member

~~Ok as of c978a83 I've~~ * replaced the ::Corrupt/::Invalid split with a chaining call to ::SetCorruptionPossible * dropped ::Invalid in favor of ::DoS (because I wanted to use Invalid for other purposes below)

~As of f759385 I have~~ * Split the DoS calls along the REJECT_CODES * Privatized DoS entirely - note this involves changing the reject code of 2 errors from 0 to REJECT_INVALID - this seemed like an oversight, but lmk if this change is problematic.

~~This PR is looking more and more inspired by #11523 :P~~

Empact force-pushed on Apr 5, 2018

jnewbery commented at 3:19 PM on April 5, 2018: member

The scope of this PR seems to have crept quite substantially! Can you update the PR title to indicate what the current branch is actually doing.

I think I like some of the new validation state interface that you've added here, but you should get the opinion of @TheBlueMatt , who's rewritten that interface here: #11639.

TheBlueMatt commented at 3:55 PM on April 5, 2018: member

Indeed, this seems to have grown into something rather duplicative with #11639 with a slightly different approach. I didn't do the removal of the "ret" argument there, but I think it (appropriately) goes further than this by removing the "DoS" field entirely (its really, really dumb that its there). Further, I think the reason list in this PR is incomplete - malleated should be a seaparate reason from Invalid, etc and I believe having a invalid-due-to-fork distinction is probably important. Not sure what the best way to move forward is here - it seems like #11639 generated no review, and now has been rewritten at least once, plus other work in this area.

Empact force-pushed on Apr 5, 2018

Empact commented at 4:34 PM on April 5, 2018: member

Fair enough! The goal was to take an incremental approach and I'd rather stick with that, so I backed out the last two commits described above, and renamed ::DoS to ::Reject, in light of @sipa's comments. @TheBlueMatt honestly I don't know the codebase well enough to evaluate #11639, trying to do something a step in that direction if possible.

Empact renamed this:
~~Drop the return argument from CValidationState::DoS and Invalid~~
Drop the return and corruptionPossible arguments from CValidationState::DoS, and rename to ::Reject
on Apr 5, 2018

Drop return argument from CValidationState::Invalid

It's false in every case.

60e0777eda

Drop return argument from CValidationState::DoS

It was always false. Note error() always returns false.

8f3a5ae31d

Drop unused default arguments to CValidationState::DoS and ::Invalid f3cd4e6583

Make the level explicit for CValidationState::Invalid a25d9b7f27

Use ::SetCorruptionPossible to declare corruption together with ::DoS

This builds the object using two calls when possiblyCorrupt argument is
set. This makes the interface more simple & explicit by making
this literal argument self-explanatory, and by not separating the
message strings.

e3721705fc

Empact force-pushed on Apr 5, 2018

Empact commented at 4:55 PM on April 5, 2018: member

Moved this back to sharing a common base with 228e3f9, for easier diffing / review.

TheBlueMatt commented at 4:59 PM on April 5, 2018: member

This seems like a great PR to make use of the scripted-diff infrastructure. re: coflicts with #11639 it might make sense to skip the "level explicit in ::Invalid" and "SetCorruptionPossible" changes (especially the SetCorruptionPossible change - I'm not convinced its the right approach - CorruptionPossible is a separate state which should be indicated as such, not a flag which should be added on top of an existing state).

Empact force-pushed on Apr 5, 2018

scripted-diff: Rename CValidationState::DoS to ::Reject

To protect against silent merge conflicts.

-BEGIN VERIFY SCRIPT-
find ./src -type f \( -name *.cpp -o -name *.h \) -exec sed -i -e 's/\bDoS(/Reject(/' {} \;
-END VERIFY SCRIPT-

9d45816f0b

Empact force-pushed on Apr 5, 2018

Empact commented at 8:45 PM on April 12, 2018: member

Planning to split this down over a few PRs to simplify review, e.g. making maximum used of scripted-diff.

Empact commented at 11:57 AM on April 13, 2018: member

Closing in favor of #12976, which attempts to incorporate all the feedback from here.

Empact closed this on Apr 13, 2018

MarcoFalke locked this on Sep 8, 2021