[Validation] Introducing Sapling transaction network connection and validations. #1940
Conversation
furszy
force-pushed
the
2020_v5_sapling_validation_first_round
branch
from
cbb9bba
to
a7f92be
Compare
|
rebased on master for #1942, making travis happy again. |
furszy
force-pushed
the
2020_v5_sapling_validation_first_round
branch
from
a7f92be
to
f293b8d
Compare
There was a problem hiding this comment.
This looks awesome, but have some talking points... mostly around the validation of the transaction size.
Aside from the comments left in-line, I see that there's no change to MAX_STANDARD_TX_SIZE.
This means that a Sapling tx, bigger than 100kB, will never be relayed, and will be valid only inside a block.
We should probably define a new constant MAX_STANDARD_SHIELDED_TX_SIZE and check standard-ness of sapling txes against that.
| // Sapling: are the sapling spends' requirements met in tx(valid anchors/nullifiers)? | ||
| if (!view.HaveShieldedRequirements(tx)) | ||
| return state.DoS(100, error("%s: spends requirements not met", __func__), | ||
| REJECT_INVALID, "bad-txns-sapling-requirements-not-met"); | ||
|
|
There was a problem hiding this comment.
(a0e9360)
This can probably be skipped here.
We are going to call CheckInputs below (line 1632), which calls CheckTxInputs (which performs this check).
There was a problem hiding this comment.
Thinking more about it... this is indeed a redundancy, and it is also true for the previous check, for regular utxos:
if (!view.HaveInputs(tx))
return state.DoS(100, error("ConnectBlock() : inputs missing/spent"),
REJECT_INVALID, "bad-txns-inputs-missingorspent");Researching it in upstream, it seems to be solved by this refactoring: bitcoin#8498.
It's probably better to leave this as is for now, and remove both redundancies in a separate PR.
furszy
force-pushed
the
2020_v5_sapling_validation_first_round
branch
from
f293b8d
to
b9150ef
Compare
|
Awesome feedback 👌👌. PR updated with all of the comments. Including the new constant definition for the shielded txs size standard-ness check. |
There was a problem hiding this comment.
Left some comment, but none is blocking. So utACK b9150ef here. Great job 🥃
| // Sapling: are the sapling spends' requirements met in tx(valid anchors/nullifiers)? | ||
| if (!view.HaveShieldedRequirements(tx)) | ||
| return state.DoS(100, error("%s: spends requirements not met", __func__), | ||
| REJECT_INVALID, "bad-txns-sapling-requirements-not-met"); | ||
|
|
There was a problem hiding this comment.
Thinking more about it... this is indeed a redundancy, and it is also true for the previous check, for regular utxos:
if (!view.HaveInputs(tx))
return state.DoS(100, error("ConnectBlock() : inputs missing/spent"),
REJECT_INVALID, "bad-txns-inputs-missingorspent");Researching it in upstream, it seems to be solved by this refactoring: bitcoin#8498.
It's probably better to leave this as is for now, and remove both redundancies in a separate PR.
| @@ -74,6 +74,7 @@ static const unsigned int DEFAULT_LIMITFREERELAY = 30; | |||
| /** The maximum size for transactions we're willing to relay/mine */ | |||
| static const unsigned int MAX_STANDARD_TX_SIZE = 100000; | |||
| static const unsigned int MAX_ZEROCOIN_TX_SIZE = 150000; | |||
| static const unsigned int MAX_STANDARD_SHIELDED_TX_SIZE = MAX_TX_SIZE_AFTER_SAPLING; | |||
There was a problem hiding this comment.
This sets the maximum size for "standard" shielded txes, to the maximum available by consensus.
In other words, there would not be any non-standard (due to its size) shielded transaction.
Not sure we really want to set it this high.
But we can discuss the proper value of this constant in another PR.
…hielded spends/outs.
…tandard-ness checks inside IsStandardTx.
furszy
force-pushed
the
2020_v5_sapling_validation_first_round
branch
from
b9150ef
to
a3c4d2d
Compare
|
nit applied 👍 + rebased on master again. Ready to get merged. |
Hopefully the last decouple from #1798.
Focused on introducing network flow connection and validations for shielded transactions (primitives and contextual checks), coins cache nullifier and anchor and a test case covering the invalid paths.
This work can be verified running 1798 functional tests suite.
sapling_wallet_nullifiers.py,sapling_wallet.py,sapling_wallet_anchorfork.py,sapling_key_import_export.pyare all running on top of this work, creating shielded txs, checking balances and generating blocks.-- A future to do in the area is the mempool validations (nullifiers) connection (stated in 1798 but a little bit of redundancy here doesn't hurt) and continue adding more test coverage for the validation area. Plus, discuss the fee topic further on its own PR, can be dynamic and not fixed as is now. --
Commits coming from 1798: