validation: Templatize ValidationState instead of subclassing by jkczyz · Pull Request #17399 · bitcoin/bitcoin

jkczyz · 2019-11-07T01:16:11Z

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

The subclassing was introduced in a27a295.

ryanofsky

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 · 2019-11-07T03:36:49Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

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

promag

Concept ACK.

src/consensus/validation.h

src/consensus/tx_check.h

laanwj · 2019-11-08T08:37:55Z

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.

ajtowns

Concept ACK

src/util/validation.h

src/consensus/validation.h

jnewbery · 2019-11-08T20:21:38Z

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

promag

Nice suggestions from @ajtowns!

src/consensus/validation.h

practicalswift · 2019-11-11T12:44:32Z

Concept ACK: new code is easier to reason about

jnewbery

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.

src/util/validation.cpp

promag

Code review ACK 0468628e8191c574d2568f5e9e7fa15dd43d80f2.

Restarted failed job on travis.

src/consensus/validation.h

jnewbery · 2019-11-13T19:41:02Z

utACK 2948ebd

jkczyz · 2019-11-30T00:58:41Z

Rebased and updated to account for #17624 in 1452949.

ariard

Code review ACK 1452949.

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

maflcko · 2019-12-20T18:00:26Z

ACK 14529499743e071ddc091e5508e1eea4532e754d 🎏

Show signature and timestamp

Signature:

Timestamp of file with hash 70e15c1a24082f6b136c95c7920ed1bc271fccb39fdfee1f98976554082edd59 -

jnewbery · 2019-12-20T23:23:57Z

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 · 2020-01-09T22:47:23Z

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

Account for changes to FormatStateMessage made in 72f3227 and 6edebac
Update use of FormatStateMessage in f9abf4a

Can be verified with:

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

jnewbery · 2020-01-10T18:43:38Z

code review ACK bf5bc260e914e2413bed4041e5089fdee03a344a

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

maflcko · 2020-02-28T19:17:44Z

ACK 10efc04 🐱

Show signature and timestamp

Signature:

Timestamp of file with hash 02d3cb1c7845b1a605977e2f801af0118c57df49376b61c77970f1807528dacf -

practicalswift · 2020-02-29T19:47:27Z

ACK 10e85d4 -- patch looks correct

ajtowns · 2020-03-01T12:53:03Z

ACK 10efc04 -- looks good to me

jonatack · 2020-03-01T14:06:33Z

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

practicalswift · 2020-03-01T20:37:46Z

post-merge ACK 10efc04

…f subclassing 10efc04 Templatize ValidationState instead of subclassing (Jeffrey Czyz) 10e85d4 Remove ValidationState's constructor (Jeffrey Czyz) 0aed17e Refactor FormatStateMessage into ValidationState (Jeffrey Czyz) Pull request description: This removes boilerplate code in the subclasses which otherwise only differ by the result type. The subclassing was introduced in a27a295. ACKs for top commit: MarcoFalke: ACK 10efc04 🐱 ajtowns: ACK 10efc04 -- looks good to me jonatack: ACK 10efc04 code review, build/tests green, nice cleanup Tree-SHA512: 765dd52dde7d49b9a5c6d99d97c96f4492673e2aed0b0604faa88db0308fa4500a26bf755cca0b896be283874096c215932e1110a2d01dc012cd36a5fce58a42

Summary: Partial backport 1/3 of core [[bitcoin/bitcoin#17399 | PR17399]]: bitcoin/bitcoin@0aed17e Test Plan: ninja all check-all Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D8326

Summary: Partial backport 2/3 of core [[bitcoin/bitcoin#17399 | PR17399]]: bitcoin/bitcoin@10e85d4 Depends on D8326. Test Plan: ninja all check Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D8327

Summary: ``` This removes boilerplate code in the subclasses which otherwise only differ by the result type. ``` Completes backport 3/3 of core [[bitcoin/bitcoin#17399 | PR17399]]: bitcoin/bitcoin@10efc04 Depends on D8327. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Subscribers: majcosta Differential Revision: https://reviews.bitcoinabc.org/D8328

…f subclassing 10efc04 Templatize ValidationState instead of subclassing (Jeffrey Czyz) 10e85d4 Remove ValidationState's constructor (Jeffrey Czyz) 0aed17e Refactor FormatStateMessage into ValidationState (Jeffrey Czyz) Pull request description: This removes boilerplate code in the subclasses which otherwise only differ by the result type. The subclassing was introduced in a27a295. ACKs for top commit: MarcoFalke: ACK 10efc04 🐱 ajtowns: ACK 10efc04 -- looks good to me jonatack: ACK 10efc04 code review, build/tests green, nice cleanup Tree-SHA512: 765dd52dde7d49b9a5c6d99d97c96f4492673e2aed0b0604faa88db0308fa4500a26bf755cca0b896be283874096c215932e1110a2d01dc012cd36a5fce58a42

jkczyz mentioned this pull request Nov 7, 2019

validation: Tidy up ValidationState interface #15921

Merged

DrahtBot added Utils/log/libs Validation labels Nov 7, 2019

ryanofsky approved these changes Nov 7, 2019

View reviewed changes

promag reviewed Nov 7, 2019

View reviewed changes

src/consensus/validation.h Outdated Show resolved Hide resolved

src/consensus/tx_check.h Outdated Show resolved Hide resolved

ajtowns reviewed Nov 8, 2019

View reviewed changes

src/util/validation.h Outdated Show resolved Hide resolved

src/consensus/validation.h Outdated Show resolved Hide resolved

jkczyz force-pushed the 2019-11-validation-state branch 2 times, most recently from 376e209 to 3f2e048 Compare November 8, 2019 21:22

DrahtBot added the Needs rebase label Nov 8, 2019

promag reviewed Nov 8, 2019

View reviewed changes

src/consensus/validation.h Outdated Show resolved Hide resolved

jkczyz force-pushed the 2019-11-validation-state branch from 3f2e048 to 0468628 Compare November 8, 2019 22:04

DrahtBot removed the Needs rebase label Nov 8, 2019

jnewbery reviewed Nov 11, 2019

View reviewed changes

src/util/validation.cpp Outdated Show resolved Hide resolved

promag reviewed Nov 11, 2019

View reviewed changes

src/consensus/validation.h Outdated Show resolved Hide resolved

jkczyz force-pushed the 2019-11-validation-state branch from 0468628 to 2948ebd Compare November 11, 2019 23:22

jkczyz mentioned this pull request Nov 22, 2019

util: Refactor message hashing into a utility function #17557

Closed

vasild mentioned this pull request Nov 24, 2019

refactor: deduplicate the message sign/verify code #17577

Merged

DrahtBot added the Needs rebase label Nov 28, 2019

jkczyz force-pushed the 2019-11-validation-state branch from 2948ebd to 1452949 Compare November 30, 2019 00:55

DrahtBot removed the Needs rebase label Nov 30, 2019

ariard reviewed Dec 6, 2019

View reviewed changes

jkczyz force-pushed the 2019-11-validation-state branch from 1452949 to a975974 Compare December 22, 2019 03:41

jkczyz force-pushed the 2019-11-validation-state branch 2 times, most recently from cb77316 to bf5bc26 Compare January 9, 2020 22:09

DrahtBot removed the Needs rebase label Jan 9, 2020

DrahtBot added the Needs rebase label Jan 29, 2020

jkczyz added 3 commits February 27, 2020 17:59

Refactor FormatStateMessage into ValidationState

0aed17e

Remove ValidationState's constructor

10e85d4

Templatize ValidationState instead of subclassing

10efc04

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

jkczyz force-pushed the 2019-11-validation-state branch from bf5bc26 to 10efc04 Compare February 28, 2020 02:24

DrahtBot removed the Needs rebase label Feb 28, 2020

maflcko merged commit 54a7ef6 into bitcoin:master Mar 1, 2020

hebasto mentioned this pull request Mar 4, 2020

qt: Use SynchronizationState enum for signals to GUI #18152

Merged

MarkLTZ mentioned this pull request Apr 4, 2020

Bitcoin PR tracking litecoinz-core/litecoinz#14

Closed

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Conversation

jkczyz commented Nov 7, 2019

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Nov 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

laanwj commented Nov 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ajtowns left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jnewbery commented Nov 8, 2019

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

practicalswift commented Nov 11, 2019

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jnewbery commented Nov 13, 2019

Uh oh!

jkczyz commented Nov 30, 2019

Uh oh!

ariard left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko commented Dec 20, 2019

Uh oh!

jnewbery commented Dec 20, 2019

Uh oh!

jkczyz commented Jan 9, 2020

Uh oh!

jnewbery commented Jan 10, 2020

Uh oh!

maflcko commented Feb 28, 2020

Uh oh!

practicalswift commented Feb 29, 2020

Uh oh!

ajtowns commented Mar 1, 2020

Uh oh!

jonatack commented Mar 1, 2020

Uh oh!

practicalswift commented Mar 1, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

DrahtBot commented Nov 7, 2019 •

edited

Loading

laanwj commented Nov 8, 2019 •

edited

Loading

ariard left a comment •

edited

Loading