validation: Templatize ValidationState instead of subclassing #17399

jkczyz commented at 1:16 AM on November 7, 2019: contributor

This removes boilerplate code in the subclasses which otherwise only differ by the result type.

The subclassing was introduced in a27a295.

DrahtBot added the label Utils/log/libs on Nov 7, 2019

DrahtBot added the label Validation on Nov 7, 2019

ryanofsky approved

ryanofsky commented at 2:14 AM on November 7, 2019: member

Code review ACK aa67e6d736fab1cf125687f3393a3eb259fb8608.

Medium concept ACK. Nice that this eliminates duplicate code. Added #includes and template function might slow down compilation a bit, though.

DrahtBot commented at 3:36 AM on November 7, 2019: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#18077 (net: Add NAT-PMP port forwarding support by hebasto)
#17828 (net: Use log categories when logging events that P2P peers can trigger arbitrarily by practicalswift)
#17812 (config, net, test: asmap functional tests and feature refinements by jonatack)
#17737 (Add ChainstateManager, remove BlockManager global by jamesob)
#17479 (Return BlockValidationState from ProcessNewBlock if CheckBlock/AcceptBlock fails by jnewbery)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

in src/consensus/validation.h:89 in aa67e6d736 outdated

  87 |      std::string m_reject_reason;
  88 |      std::string m_debug_message;
  89 | -protected:
  90 | -    void Invalid(const std::string &reject_reason="",
  91 | +public:
  92 | +    ValidationState() : m_mode(MODE_VALID) {}

promag commented at 10:28 AM on November 7, 2019:

nit, in L85 do } m_mode{MODE_VALID}; and drop constructor?

jkczyz commented at 6:30 PM on November 7, 2019:

The diff here is a bit misleading as I'm actually moving Invalid to the public section and not modifying the constructor. Is the convention to put this in a separate a commit? Or would it be considered outside the scope of the PR?

promag commented at 9:26 PM on November 8, 2019:

Right, but the constructor could still be dropped.

jkczyz commented at 10:16 PM on November 8, 2019:

Understood. But that doesn't answer either of my questions... 🤔

promag commented at 3:37 PM on November 11, 2019:

IMHO could be added in "Templatize ValidationState instead of subclassing", but no big deal.

jkczyz commented at 11:24 PM on November 11, 2019:

Ended up doing this separately in 7470037 as it gives cleaner diffs.

in src/consensus/tx_check.h:8 in aa67e6d736 outdated

   4 | @@ -5,6 +5,8 @@
   5 |  #ifndef BITCOIN_CONSENSUS_TX_CHECK_H
   6 |  #define BITCOIN_CONSENSUS_TX_CHECK_H
   7 |  
   8 | +#include <consensus/validation.h>

promag commented at 10:33 AM on November 7, 2019:

You can still forward declare TxValidationState and BlockValidationState where needed and avoid the issues pointed by @ryanofsky?

jkczyz commented at 6:28 PM on November 7, 2019:

This causes a compilation error:

In file included from consensus/tx_check.cpp:8:
./consensus/validation.h:115:7: error: typedef redefinition with different types ('ValidationState<TxValidationResult>' vs 'TxValidationState')
using TxValidationState = ValidationState<TxValidationResult>;
      ^
./consensus/tx_check.h:16:7: note: previous definition is here
class TxValidationState;

I'd have to forward declare the full template type and enum then use that in the function declaration.

enum class TxValidationResult;
template <typename Result>
class ValidationState;

bool CheckTransaction(const CTransaction& tx, ValidationState<TxValidationResult>& state);

Seems a bit verbose and would mismatch the function definition. FWIW, here are some pros and cons of forward declaring that I'm aware of.

promag commented at 10:34 AM on November 7, 2019: member

Concept ACK.

laanwj commented at 8:37 AM on November 8, 2019: member

Concept ACK.

Added #includes and template function might slow down compilation a bit, though.

Yes, although this is a quite straightforward use of templates, no boost nested template horrors, so I hope it's not too bad.

in src/util/validation.h:15 in aa67e6d736 outdated

  13 | +#include <string>
  14 |  
  15 |  /** Convert ValidationState to a human-readable message for logging */
  16 | -std::string FormatStateMessage(const ValidationState &state);
  17 | +template <typename Result>
  18 | +std::string FormatStateMessage(const ValidationState<Result>& state)

ajtowns commented at 11:35 AM on November 8, 2019:

Could move this into ValidationState directly, and make it

std::string FormatStateMessage() const { return m_reject_reason + (m_debug_message.empty() ? "" : ", " + m_debug_message); }

jkczyz commented at 10:15 PM on November 8, 2019:

Done in 7239d01 but named it ToString since #16688 uses it for valid states as well.

in src/consensus/validation.h:115 in aa67e6d736 outdated

 140 | -        ValidationState::Invalid(reject_reason, debug_message);
 141 | -        return false;
 142 | -    }
 143 | -    BlockValidationResult GetResult() const { return m_result; }
 144 | -};
 145 | +using TxValidationState = ValidationState<TxValidationResult>;

ajtowns commented at 11:38 AM on November 8, 2019:

I think you can do class TxValidationState : ValidationState<TxValidationResult> { }; and still be able to use forward declarations class TxValidationState; fwiw.

jkczyz commented at 10:15 PM on November 8, 2019:

Good idea! Done in 0468628.

ajtowns commented at 11:39 AM on November 8, 2019: member

Concept ACK

jnewbery commented at 8:21 PM on November 8, 2019: member

Concept ACK. Will review code once @ajtowns's comments are addressed.

jkczyz force-pushed on Nov 8, 2019

DrahtBot added the label Needs rebase on Nov 8, 2019

promag commented at 9:27 PM on November 8, 2019: member

Nice suggestions from @ajtowns!

jkczyz force-pushed on Nov 8, 2019

DrahtBot removed the label Needs rebase on Nov 8, 2019

practicalswift commented at 12:44 PM on November 11, 2019: contributor

Concept ACK: new code is easier to reason about

in src/util/validation.cpp:8 in 0468628e81 outdated

  14 | -    return strprintf("%s%s",
  15 | -        state.GetRejectReason(),
  16 | -        state.GetDebugMessage().empty() ? "" : ", "+state.GetDebugMessage());
  17 | -}
  18 | -
  19 |  const std::string strMessageMagic = "Bitcoin Signed Message:\n";

jnewbery commented at 2:48 PM on November 11, 2019:

It seems a bit sad to leave this const all by itself in this implementation file. Perhaps move it to the header file and change to a constexpr? Bonus points for renaming util/validation.h to util/strings.h since it isn't anything to do with validation any more, and I can't find anywhere that would be a better home for this const.

MarcoFalke commented at 4:06 PM on November 11, 2019:

I don't thing std::string can be constexpr. Also, moving it to the header file might inline it in every translation unit. Probably doesn't matter for a short string like this, but no need to change existing code to make it worse.

jnewbery commented at 4:49 PM on November 11, 2019:

You're right. std::string can't be constexpr.

jkczyz commented at 11:14 PM on November 11, 2019:

Looking at the references to strMessageMagic, they are always of the form:

CHashWriter ss(SER_GETHASH, 0);
ss << strMessageMagic;
ss << strMessage;

And then the result of its GetHash is passed to either SignCompact or RecoverCompact, and the variable ss is never used again.

So the above code could be refactored into a function where strMessageMagic is just an implementation detail. Given that, is there a more appropriate place for it? What does ss stand for here?

jnewbery commented at 11:19 PM on November 11, 2019:

What does ss stand for here?

Not sure. Serialization stream?

jnewbery commented at 11:22 PM on November 11, 2019:

(there have been CDataStream ss variables in the codebase since Satoshi era)

jkczyz commented at 12:41 AM on November 12, 2019:

Perhaps I could move that boilerplate code to util/hash.h as HashMessage?

uint256 HashMessage(const std::string& message);

Open to more suitable naming for the file and function. Note that util/string.h already exists for string joining, so the plural version may be confusing.

Would likely leave this to a follow-up PR given I'd be refactoring call sites.

jnewbery commented at 7:38 PM on November 13, 2019:

Open to more suitable naming for the file and function

Perhaps /util/message.h?

Would likely leave this to a follow-up PR given I'd be refactoring call sites.

Yeah, anything other than a simple rename should be left as a follow-up PR.

jkczyz commented at 6:28 AM on November 22, 2019:

Opened #17557 with the refactor.

jkczyz commented at 6:17 PM on February 28, 2020:

Rebased and was able to remove util/validation.h and util/validation.cpp now that #17577 is merged.

jnewbery commented at 2:56 PM on November 11, 2019: member

Tested ACK 0468628e8191c574d2568f5e9e7fa15dd43d80f2. Nice tidy up!

One suggestion for getting rid of a now almost-empty file. Can be done in this PR or a follow-up.

promag commented at 3:57 PM on November 11, 2019: member

Code review ACK 0468628e8191c574d2568f5e9e7fa15dd43d80f2.

Restarted failed job on travis.

jkczyz force-pushed on Nov 11, 2019

jnewbery commented at 7:41 PM on November 13, 2019: member

utACK 2948ebd29d0a53f11aefc8546bcac4125c726cc6

DrahtBot added the label Needs rebase on Nov 28, 2019

jkczyz force-pushed on Nov 30, 2019

jkczyz commented at 12:58 AM on November 30, 2019: contributor

Rebased and updated to account for #17624 in 1452949.

DrahtBot removed the label Needs rebase on Nov 30, 2019

ariard commented at 5:25 PM on December 6, 2019: member

Code review ACK 1452949.

About the increase compilation time concern, on my system compare to master it's even a bit slightly slightly faster.

MarcoFalke commented at 6:00 PM on December 20, 2019: member

ACK 14529499743e071ddc091e5508e1eea4532e754d 🎏

<details><summary>Show signature and timestamp</summary>

Signature:

Timestamp of file with hash 70e15c1a24082f6b136c95c7920ed1bc271fccb39fdfee1f98976554082edd59 -

</details>

jnewbery commented at 11:23 PM on December 20, 2019: member

utACK 14529499743e071ddc091e5508e1eea4532e754d

Since the initialization relies on [TX|BLOCK]_RESULT_UNSET being 0, what do you think of:

--- a/src/consensus/validation.h
+++ b/src/consensus/validation.h
@@ -16,7 +16,7 @@
   * provider of the transaction should be banned/ignored/disconnected/etc.
   */
 enum class TxValidationResult {
-    TX_RESULT_UNSET,         //!< initial value. Tx has not yet been rejected
+    TX_RESULT_UNSET = 0,     //!< initial value. Tx has not yet been rejected. Must be zero/first defined enumerator for default initialization.
     TX_CONSENSUS,            //!< invalid by consensus rules
     /**
      * Invalid by a change to consensus rules more recent than SegWit.
@@ -50,7 +50,7 @@ enum class TxValidationResult {
   * useful for some other use-cases.
   */
 enum class BlockValidationResult {
-    BLOCK_RESULT_UNSET,      //!< initial value. Block has not yet been rejected
+    BLOCK_RESULT_UNSET = 0,  //!< initial value. Block has not yet been rejected. Must be zero/first defined enumerator for default initialization.
     BLOCK_CONSENSUS,         //!< invalid by consensus rules (excluding any below reasons)
     /**
      * Invalid by a change to consensus rules more recent than SegWit.

jkczyz force-pushed on Dec 22, 2019

jkczyz commented at 3:46 AM on December 22, 2019: contributor

utACK 1452949

Since the initialization relies on [TX|BLOCK]_RESULT_UNSET being 0, what do you think of:

--- a/src/consensus/validation.h
+++ b/src/consensus/validation.h
@@ -16,7 +16,7 @@
   * provider of the transaction should be banned/ignored/disconnected/etc.
   */
 enum class TxValidationResult {
-    TX_RESULT_UNSET,         //!< initial value. Tx has not yet been rejected
+    TX_RESULT_UNSET = 0,     //!< initial value. Tx has not yet been rejected. Must be zero/first defined enumerator for default initialization.
     TX_CONSENSUS,            //!< invalid by consensus rules
     /**
      * Invalid by a change to consensus rules more recent than SegWit.
@@ -50,7 +50,7 @@ enum class TxValidationResult {
   * useful for some other use-cases.
   */
 enum class BlockValidationResult {
-    BLOCK_RESULT_UNSET,      //!< initial value. Block has not yet been rejected
+    BLOCK_RESULT_UNSET = 0,  //!< initial value. Block has not yet been rejected. Must be zero/first defined enumerator for default initialization.
     BLOCK_CONSENSUS,         //!< invalid by consensus rules (excluding any below reasons)
     /**
      * Invalid by a change to consensus rules more recent than SegWit.

Good call. Pushed a975974. @promag @ariard @MarcoFalke You can verify the change with git range-diff abb30de63 145294997 a975974f4.

jnewbery commented at 3:38 PM on December 22, 2019: member

ACK a975974

DrahtBot added the label Needs rebase on Jan 2, 2020

jkczyz force-pushed on Jan 3, 2020

jkczyz commented at 8:40 PM on January 3, 2020: contributor

Rebased

jnewbery commented at 9:00 PM on January 3, 2020: member

reACK 2afff87a88a3f539cece488f927386a6e106cdc2

DrahtBot removed the label Needs rebase on Jan 3, 2020

ariard commented at 11:16 PM on January 3, 2020: member

ACK 2afff87, only change since 1452949 is default initialization first member of TxValidationResult/BlockValidationResult enums to 0.

MarcoFalke deleted a comment on Jan 7, 2020

DrahtBot added the label Needs rebase on Jan 9, 2020

jkczyz force-pushed on Jan 9, 2020

jkczyz commented at 10:47 PM on January 9, 2020: contributor

Rebased to account for changes in #16688. Specifically, pushed 7bb194ba252191b531803ad2952c984c70d192da to:

Account for changes to FormatStateMessage made in 72f3227c83810936e7a334304e5fd7c6dab8e91b and 6edebacb2191373e76d79a4972d6192300976096
Update use of FormatStateMessage in f9abf4ab6d3d3e4d4b7e90723020b5422a141a6f

Can be verified with:

git range-diff 17e14ac..2afff87 e7f8450..bf5bc26

DrahtBot removed the label Needs rebase on Jan 9, 2020

jnewbery commented at 6:43 PM on January 10, 2020: member

code review ACK bf5bc260e914e2413bed4041e5089fdee03a344a

DrahtBot added the label Needs rebase on Jan 29, 2020

Refactor FormatStateMessage into ValidationState 0aed17ef28

Remove ValidationState's constructor 10e85d4adc

Templatize ValidationState instead of subclassing

This removes boilerplate code in the subclasses which otherwise only
differ by the result type.

10efc0487c

jkczyz force-pushed on Feb 28, 2020

DrahtBot removed the label Needs rebase on Feb 28, 2020

MarcoFalke commented at 7:17 PM on February 28, 2020: member

ACK 10efc0487c442bccb0e4a9ac29452af1592a3cf2 🐱

<details><summary>Show signature and timestamp</summary>

Signature:

Timestamp of file with hash 02d3cb1c7845b1a605977e2f801af0118c57df49376b61c77970f1807528dacf -

</details>

practicalswift commented at 7:47 PM on February 29, 2020: contributor

ACK 10e85d4adc9b7dbbda63e00195e0a962f51e4d2c -- patch looks correct

ajtowns commented at 12:53 PM on March 1, 2020: member

ACK 10efc0487c442bccb0e4a9ac29452af1592a3cf2 -- looks good to me

jonatack commented at 2:06 PM on March 1, 2020: member

ACK 10efc048 code review, build/tests green, nice cleanup

MarcoFalke merged this on Mar 1, 2020

MarcoFalke closed this on Mar 1, 2020

practicalswift commented at 8:37 PM on March 1, 2020: contributor

post-merge ACK 10efc0487c442bccb0e4a9ac29452af1592a3cf2

sidhujag referenced this in commit 12748b0c7c on Mar 2, 2020

deadalnix referenced this in commit 46a3c8ef67 on Nov 9, 2020

jasonbcox referenced this in commit 0bc9d84a6c on Nov 9, 2020

jasonbcox referenced this in commit 0f17907934 on Nov 9, 2020

sidhujag referenced this in commit c07d8cbb4c on Nov 10, 2020

DrahtBot locked this on Feb 15, 2022