[Refactor] Mix refactoring in preparation for DMN lists #2270

random-zebra · 2021-03-25T21:02:49Z

Extracted from #2267.
This is a collection of small refactorings around the validation code, which will make future additions easier to review.
See individual commit's description.

src/validation.cpp

src/serialize.h

src/script/interpreter.cpp

src/validation.cpp

furszy · 2021-04-05T17:37:57Z

src/wallet/wallet.cpp

I'm thinking about this, why this is needed?

In the FundTransaction flow, the user is providing the tx in hex format and the node is parsing it. If there would be an extra payload or sapling data inside the tx, this line GetSerializeSize(tx) + extraSize will sum them twice as GetSerializeSize calls internally to the tx serialization method which is already taking into account the two fields.

In the FundTransaction flow, the user is providing the tx in hex format and the node is parsing it.

Mhm... there is some confusion.
This is the wallet function. There is no hex format, or parsing, directly involved here.
A generic caller to CWallet::FundTransaction provides the transaction to be funded, as a CMutableTransaction tx (not an hex string).

The outputs of this mutable tx are used to fill vecSend, passed to CWallet::CreateTransaction.
With the input data, CreateTransaction creates a mutable transaction object txNew.

Since, in order to select enough inputs, the transaction fee must be known, the serialized size is calculated inside CreateTransaction.
This calculation is obviously done on the newly created CMutableTransaction (the txNew object), which has no extra payload accounted for.

Also, while currently CWallet::FundTransaction is called exclusively from the RPC command fundrawtransaction (after parsing the user provided hex), soon there will be more RPCs (which involve no parsing of raw txes) relying on this wallet function.

this line GetSerializeSize(tx) + extraSize will sum them twice

No. You can easily verify, trying to remove this on #2267.
The tests will fail due to fee too low.
Also note that txNew is being serialized (not the mutable tx passed to FundTransaction).

src/consensus/tx_verify.cpp

src/chainparams.cpp

src/consensus/tx_verify.cpp

Also rename 'tiertwo' folder to 'evo'

>>> Inspired by bitcoin#11864 (+bitcoin#10784) Since CreateTransaction still needs cs_main we need to obtain both locks in FundTransaction, to preserve locking order. After introducing the chain interface (bitcoin#14437) we can change the LOCK2(cs_main, cs_wallet) to just LOCK(cs_wallet)

Since we will need the current tip index in CheckProRegTx to check for duplicate unique-properties in the deterministic mn list.

the boolean argument fJustCheck in ProcessSpecialTxsInBlock will be used later, after the connection to the deterministic manager

Also don't expose it (as it is only called by ContextualCheckZerocoinTx) and cleanup logs with __func__

furszy

Looking better now 👌 , nice work.
ACK b408de3 .

random-zebra · 2021-04-08T11:48:50Z

Would be good to complete the review and merge this one (opened 2 weeks ago), so we can keep going with the DMN stuff.

Fuzzbawls

Code ACK b408de3

random-zebra · 2021-04-11T07:57:05Z

Merging...

c27bbed [Cleanup] functions impl: branch on new line (random-zebra) 8850b3e [Refactoring] Make CEvoDB mutex public (random-zebra) 304fc46 Take memory used by CEvoDB/CDBTransaction into account when flushing (random-zebra) ed0d4f9 Track memory usage in CDBTransaction and CEvoDB (random-zebra) f259774 Implement CDBTransactionIterator (Alexander Block) 51920cb Change CDBTransaction to compare keys by their serialized form (Alexander Block) 7094a60 Support passing CDataStream as key into CDBWrapper/CDBBatch/CDBIterator (Alexander Block) cdd44d4 [Cleanup] Drop unneeded casts (random-zebra) 65b8ddc [DB] Specialize CScopedDBTransaction for evo database (random-zebra) 34a7541 [Refactoring] Properly add explicit specifier where appropriate (random-zebra) 6a36968 Implement 2-stage commit for CEvoDB to avoid inconsistencies (Alexander Block) e6c7efe [Refactoring] Let Commit() return void (random-zebra) fec56b0 [DB] Ensure evoDB consistency by storing best block hash (random-zebra) be85c9a [Refactoring] migrate evoDb to unique pointer (random-zebra) cab50d3 [Tests] Fix functional test suite with new directory 'evodb' (random-zebra) ad7f5d7 [DB][BUG] Add virtual destructor for KeyValueHolder (random-zebra) d6c56d1 Introduce CEvoDB for all evo related things, e.g. DIP3 (Alexander Block) 2496933 Implement CDBTransaction and CScopedDBTransaction (Alexander Block) da6a2c1 [Build] CMake: Introduce evo headers (random-zebra) f3a7cfb [Trivial] Remove unused variable in ProcessNewBlock (random-zebra) Pull request description: Extracted from #2267. This introduces the new database, used for all DMN-related things, and updates the dbWrapper. Builds on top of: - [x] #2269 - [x] #2270 ACKs for top commit: Fuzzbawls: Code ACK c27bbed furszy: Code ACK c27bbed. Tree-SHA512: a93b0c96759b9493659d2e5c69d2fa9d7dbcb260b7f7ec4d2e7db2d1f2a4561f9548bbe4db07272792b5d6dbf33fd8e7c84f04d958781e6d9bb3fb1fdcbf2ea1

random-zebra added Validation Refactoring labels Mar 25, 2021

random-zebra added this to the 6.0.0 milestone Mar 25, 2021

random-zebra self-assigned this Mar 25, 2021

random-zebra mentioned this pull request Mar 25, 2021

[DB] Introduce EVO database #2271

Merged

2 tasks

random-zebra force-pushed the 202103-mix-refactoring branch from 73ca597 to 9a1948e Compare March 25, 2021 21:37

random-zebra mentioned this pull request Mar 25, 2021

[Doc] Introducing Deterministic Masternodes #2267

Merged

furszy reviewed Mar 31, 2021

View reviewed changes

src/validation.cpp Outdated Show resolved Hide resolved

random-zebra force-pushed the 202103-mix-refactoring branch from 9a1948e to 23e8c3e Compare March 31, 2021 20:17

furszy reviewed Mar 31, 2021

View reviewed changes

src/serialize.h Outdated Show resolved Hide resolved

src/script/interpreter.cpp Outdated Show resolved Hide resolved

random-zebra force-pushed the 202103-mix-refactoring branch from 23e8c3e to a361805 Compare March 31, 2021 23:05

furszy reviewed Apr 5, 2021

View reviewed changes

random-zebra force-pushed the 202103-mix-refactoring branch from a361805 to be31458 Compare April 5, 2021 19:59

furszy reviewed Apr 5, 2021

View reviewed changes

src/consensus/tx_verify.cpp Outdated Show resolved Hide resolved

random-zebra force-pushed the 202103-mix-refactoring branch from be31458 to 0b9a21f Compare April 5, 2021 21:32

furszy reviewed Apr 6, 2021

View reviewed changes

src/chainparams.cpp Outdated Show resolved Hide resolved

furszy suggested changes Apr 6, 2021

View reviewed changes

src/consensus/tx_verify.cpp Outdated Show resolved Hide resolved

furszy self-requested a review April 6, 2021 21:42

random-zebra added 12 commits April 7, 2021 12:14

[MOVE] rename files specialtx_validation.* --> specialtx.*

6267534

Also rename 'tiertwo' folder to 'evo'

[Refactor] Introduce GetUTXO* functions relying on pcoinsTip

a0a5170

[Tests] Fix proper sapling tx version in unit tests

211b36d

[Core] Add GetSerializeSizeNetwork for generic (non-array) Optionals

69662f8

[RPC] Return full debug message for ATMP failures in sendrawtransaction

e288fd6

[Trivial][Cleanup] Remove unused variable in ScanForWalletTransactions

517fe0c

[Wallet] Account for extra payload sizes in Fund/CreateTransaction

9ef8031

[Cleanup] Remove fSaplingActive from CheckSpecialTx

e1eb3f5

[Tests][Trivial] Remove annoying warning for unintended optimization

dab3830

[Refactoring] move sapling contextual checks out of CheckTransaction

11d2025

[Refactoring] unify contextual checks for txes

bfd3fe4

random-zebra force-pushed the 202103-mix-refactoring branch from 0b9a21f to 65b6bd3 Compare April 7, 2021 10:40

random-zebra added 4 commits April 7, 2021 12:50

[Refactoring] move zerocoin contextual checks out of CheckTransaction

63a87f9

[Refactoring] Move CheckSpecialTx out of CheckTransaction

eba2377

Since we will need the current tip index in CheckProRegTx to check for duplicate unique-properties in the deterministic mn list.

[Refactoring][Consensus] Connect SpecialTx processing to validation code

34882e6

the boolean argument fJustCheck in ProcessSpecialTxsInBlock will be used later, after the connection to the deterministic manager

[Cleanup] Remove unused boolean arguments in CheckZerocoinSpend

50a55b7

Also don't expose it (as it is only called by ContextualCheckZerocoinTx) and cleanup logs with __func__

random-zebra force-pushed the 202103-mix-refactoring branch 3 times, most recently from e263d9c to 3490971 Compare April 7, 2021 13:56

[Refactoring] non-contextual checks for special txes in CheckBlock

b408de3

random-zebra force-pushed the 202103-mix-refactoring branch from 3490971 to b408de3 Compare April 7, 2021 14:33

furszy approved these changes Apr 7, 2021

View reviewed changes

random-zebra requested a review from Fuzzbawls April 8, 2021 11:46

Fuzzbawls approved these changes Apr 11, 2021

View reviewed changes

random-zebra merged commit f461766 into PIVX-Project:master Apr 11, 2021

This was referenced Apr 16, 2021

[TierTwo] Deterministic masternode lists #2273

Merged

Fuzzing framework support #2252

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[Refactor] Mix refactoring in preparation for DMN lists #2270

[Refactor] Mix refactoring in preparation for DMN lists #2270

Uh oh!

random-zebra commented Mar 25, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

furszy Apr 5, 2021

Uh oh!

random-zebra Apr 5, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

furszy left a comment

Uh oh!

random-zebra commented Apr 8, 2021

Uh oh!

Fuzzbawls left a comment

Uh oh!

random-zebra commented Apr 11, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

[Refactor] Mix refactoring in preparation for DMN lists #2270

[Refactor] Mix refactoring in preparation for DMN lists #2270

Uh oh!

Conversation

random-zebra commented Mar 25, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

furszy Apr 5, 2021

Choose a reason for hiding this comment

Uh oh!

random-zebra Apr 5, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

furszy left a comment

Choose a reason for hiding this comment

Uh oh!

random-zebra commented Apr 8, 2021

Uh oh!

Fuzzbawls left a comment

Choose a reason for hiding this comment

Uh oh!

random-zebra commented Apr 11, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants