-
Notifications
You must be signed in to change notification settings - Fork 219
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
docs: add block/transaction weights to RFC #3368
docs: add block/transaction weights to RFC #3368
Conversation
2d2023f
to
402cf5f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. The only thing I think we need to think about is moving the script weight to the output if we can. We don't want to penalize a person for spending an output.
655f604
to
cf5b45c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
| - Per input | ~196 | 1 | Excl. script field | | ||
| Kernel: | | | | | ||
| - Per kernel | 112 | 3 | Excess signature verification | | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For context, the original weightings were calculated as follows
- Closely correlated to the space taken up on the blockchain
- Smallest element (input) has a weight of 1. Everything else scaled to that.
- Max block weight set to have max block sizes of ±1MB
Computation time wasn't taken into account, but this is an interesting idea.
Now clearly, kernels are the smallest contributor to the block, so you might want to set that weight to 1
Inputs are almost 2. With script some script inputs it'll be larger than 2. (You didn't indicate how you got to 196 bytes).
Outputs are now 7. But there is an argument for making this even larger since we know that RP validation is the most expensive part of tx validation currently.
With scripts, we could go flat rate per opcode, or we could go full ethereum and assign a weight for each opcode. a PushZero
is basically free, both in space and time, while CheckMultiSig(16,32,...)
is computationally and spatially expensive.
A middle ground is to assign something like 1.5 grams per byte of the serialised script to recover some of the computationally expensive operations (the exact number will require some benchmarking and investigation).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's great thanks, pretty much exactly how I thought about the original weightings and I got the same values [1,2,7] as you given the byte sizes (quickly summed up from the respective structs, pk compressed only EDIT: used OutputFeatures that are not implemented yet from #3340) - though I decided not to deviate from current weightings without some discussion.
I suppose the challenge is how to go about deciding on values for these while being cognisant of relative impacts on total weights, unintentional incentives, complexity, performance etc. My feeling is that TariScript serialisation is likely fast enough to use during verification (or perhaps even use raw bytes before deserialisation), avoids the task of maintaining weightings per OpCode and we can target approx. the same number of NoOp transactions per 1MB block - with larger scripts/asset reg. eating into the max total block weight as appropriate. I suspect the factor per byte will be < 1 since e.g. a single ~32 byte PushPubkey would take the same weight as ~4.5 outputs (or 2.5 if outputs are weighted 13) - or per output etc weightings should be made larger.
I'll investigate and run the numbers in a spreadsheet and see what I can come up with
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, it's < 1 weight factor. I was implicitly suggesting 1.5 times per byte. So the weight would be more like 1.5/112 = 0.014
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using computation is a good metric for the scripts, but we need to look at the weights carefully. Although a kernel is the smallest unit they don't get pruned. We want to force the user to use as few as possible kernels in his transaction as they stay forever and never get pruned. Rangeproofs are big and computationally expensive, that's why we should weigh them big and they are. We want to spend their inputs, so we weigh them as little as possible.
I think it's important to keep in mind why we use weights in the blocks.
1: It's to keep the block to a certain size.
2: We use the weights to pay fees proportionately.
But with point 2, we also have to remember we use fees to incentivize users to use the correct behavior. What is the best ideal case: Spend as many inputs as possible, make as few as possible outputs, use as few as possible kernels, and use as fast/small scripts as possible. The fees and in turn the weights should follow this.
I'm not too fussed about the specifics here, but I would suggest only bringing performance into account when there is an expensive lookup operation. Searching the DB or index far outweighs the CPU task on the scripts. I'd suggest:
Worth noting that the current implementation copies the TransactionOutput (excl rangeproof) into the TransactionInput, which is to simplify implementation. The actual storage of the input during block sync and storage needs only include a reference to the output, not the full script + features etc. So the size of input may be incorrect here |
@mikethetike Yeah agree with that, inputs should be "underweighted" to prevent a disincentive to spend and kernels "overweighted" because they cannot be pruned. What if we did this: The target block size is 1MiB (maybe should slightly increase), we want that to be represented in 50000 grams (max block weight) so 21 bytes per gram.
The weightings should also take some other considerations:
Given these weights you would be able to fit in roughly 950 "normal" transactions in a block EDIT: a gram has essentially become 5x more fine grained (113/21), so fee per gram should be ~5x smaller In any case, I'll write up a basic version of this where we just take a factor of the bytes for TS and output features to get the gram weight and we can tweak the numbers from there |
Sounds good |
Add `Block/Transaction Weight` section to Base node RFC
@mikethetike Has suggested (and I agree) that 21 bytes per gram isn't ideal, 16/32 would be preferable. |
cf5b45c
to
cf624a4
Compare
ba96e09
to
282dfa7
Compare
I just thought you were a big Sean Penn fan https://www.imdb.com/title/tt0315733/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
- adds consensus encoding for tari script and output features - igor: tx weight takes TariScript and OutputFeatures metadata into account as per tari-project#3368 - igor: weights updated as per tari-project#3368 - add `TransactionWeight` parameters for weatherwax and igor networks and set them in consensus constants - set max block weight for igor (+95% adjust to allow similar number of max inputs/outputs/kernels ~2Mb) - decouple chain_storage from consensus_contstants module to allow wallet to use it - minor simlification of wallet coin split logic - wallet and base node tests updated as needed
- adds consensus encoding for tari script and output features - igor: tx weight takes TariScript and OutputFeatures metadata into account as per tari-project#3368 - igor: weights updated as per tari-project#3368 - add `TransactionWeight` parameters for weatherwax and igor networks and set them in consensus constants - set max block weight for igor (+95% adjust to allow similar number of max inputs/outputs/kernels ~2Mb) - decouple chain_storage from consensus_contstants module to allow wallet to use it - minor simlification of wallet coin split logic - wallet and base node tests updated as needed
- adds consensus encoding for tari script and output features - igor: tx weight takes TariScript and OutputFeatures metadata into account as per tari-project#3368 - igor: weights updated as per tari-project#3368 - add `TransactionWeight` parameters for weatherwax and igor networks and set them in consensus constants - set max block weight for igor (+95% adjust to allow similar number of max inputs/outputs/kernels ~2Mb) - decouple chain_storage from consensus_contstants module to allow wallet to use it - minor simlification of wallet coin split logic - wallet and base node tests updated as needed
- adds consensus encoding for tari script and output features - igor: tx weight takes TariScript and OutputFeatures metadata into account as per tari-project#3368 - igor: weights updated as per tari-project#3368 - add `TransactionWeight` parameters for weatherwax and igor networks and set them in consensus constants - set max block weight for igor (+95% adjust to allow similar number of max inputs/outputs/kernels ~2Mb) - decouple chain_storage from consensus_contstants module to allow wallet to use it - minor simlification of wallet coin split logic - wallet and base node tests updated as needed
- adds consensus encoding for tari script and output features - igor: tx weight takes TariScript and OutputFeatures metadata into account as per tari-project#3368 - igor: weights updated as per tari-project#3368 - add `TransactionWeight` parameters for weatherwax and igor networks and set them in consensus constants - set max block weight for igor (+95% adjust to allow similar number of max inputs/outputs/kernels ~2Mb) - decouple chain_storage from consensus_contstants module to allow wallet to use it - minor simlification of wallet coin split logic - wallet and base node tests updated as needed
- adds consensus encoding for tari script and output features - igor: tx weight takes TariScript and OutputFeatures metadata into account as per tari-project#3368 - igor: weights updated as per tari-project#3368 - add `TransactionWeight` parameters for weatherwax and igor networks and set them in consensus constants - set max block weight for igor (+95% adjust to allow similar number of max inputs/outputs/kernels ~2Mb) - decouple chain_storage from consensus_contstants module to allow wallet to use it - minor simlification of wallet coin split logic - wallet and base node tests updated as needed
- adds consensus encoding for tari script and output features - igor: tx weight takes TariScript and OutputFeatures metadata into account as per tari-project#3368 - igor: weights updated as per tari-project#3368 - add `TransactionWeight` parameters for weatherwax and igor networks and set them in consensus constants - set max block weight for igor (+95% adjust to allow similar number of max inputs/outputs/kernels ~2Mb) - decouple chain_storage from consensus_contstants module to allow wallet to use it - minor simlification of wallet coin split logic - wallet and base node tests updated as needed
- adds consensus encoding for tari script and output features - igor: tx weight takes TariScript and OutputFeatures metadata into account as per tari-project#3368 - igor: weights updated as per tari-project#3368 - add `TransactionWeight` parameters for weatherwax and igor networks and set them in consensus constants - set max block weight for igor (+95% adjust to allow similar number of max inputs/outputs/kernels ~2Mb) - decouple chain_storage from consensus_contstants module to allow wallet to use it - minor simlification of wallet coin split logic - wallet and base node tests updated as needed
- adds consensus encoding for tari script and output features - igor: tx weight takes TariScript and OutputFeatures metadata into account as per tari-project#3368 - igor: weights updated as per tari-project#3368 - add `TransactionWeight` parameters for weatherwax and igor networks and set them in consensus constants - set max block weight for igor (+95% adjust to allow similar number of max inputs/outputs/kernels ~2Mb) - decouple chain_storage from consensus_contstants module to allow wallet to use it - minor simlification of wallet coin split logic - wallet and base node tests updated as needed
- adds consensus encoding for tari script and output features - igor: tx weight takes TariScript and OutputFeatures metadata into account as per tari-project#3368 - igor: weights updated as per tari-project#3368 - add `TransactionWeight` parameters for weatherwax and igor networks and set them in consensus constants - set max block weight for igor (+95% adjust to allow similar number of max inputs/outputs/kernels ~2Mb) - decouple chain_storage from consensus_contstants module to allow wallet to use it - minor simlification of wallet coin split logic - wallet and base node tests updated as needed
- adds consensus encoding for tari script and output features - igor: tx weight takes TariScript and OutputFeatures metadata into account as per tari-project#3368 - igor: weights updated as per tari-project#3368 - add `TransactionWeight` parameters for weatherwax and igor networks and set them in consensus constants - set max block weight for igor (+95% adjust to allow similar number of max inputs/outputs/kernels ~2Mb) - decouple chain_storage from consensus_contstants module to allow wallet to use it - minor simlification of wallet coin split logic - wallet and base node tests updated as needed
- adds consensus encoding for tari script and output features - igor: tx weight takes TariScript and OutputFeatures metadata into account as per tari-project#3368 - igor: weights updated as per tari-project#3368 - add `TransactionWeight` parameters for weatherwax and igor networks and set them in consensus constants - set max block weight for igor (+95% adjust to allow similar number of max inputs/outputs/kernels ~2Mb) - decouple chain_storage from consensus_contstants module to allow wallet to use it - minor simlification of wallet coin split logic - wallet and base node tests updated as needed
- adds consensus encoding for tari script and output features - igor: tx weight takes TariScript and OutputFeatures metadata into account as per tari-project#3368 - igor: weights updated as per tari-project#3368 - add `TransactionWeight` parameters for weatherwax and igor networks and set them in consensus constants - set max block weight for igor (+95% adjust to allow similar number of max inputs/outputs/kernels ~2Mb) - decouple chain_storage from consensus_contstants module to allow wallet to use it - minor simlification of wallet coin split logic - wallet and base node tests updated as needed
- adds consensus encoding for tari script and output features - igor: tx weight takes TariScript and OutputFeatures metadata into account as per tari-project#3368 - igor: weights updated as per tari-project#3368 - add `TransactionWeight` parameters for weatherwax and igor networks and set them in consensus constants - set max block weight for igor (+95% adjust to allow similar number of max inputs/outputs/kernels ~2Mb) - decouple chain_storage from consensus_contstants module to allow wallet to use it - minor simlification of wallet coin split logic - wallet and base node tests updated as needed
- adds consensus encoding for tari script and output features - igor: tx weight takes TariScript and OutputFeatures metadata into account as per tari-project#3368 - igor: weights updated as per tari-project#3368 - add `TransactionWeight` parameters for weatherwax and igor networks and set them in consensus constants - set max block weight for igor (+95% adjust to allow similar number of max inputs/outputs/kernels ~2Mb) - decouple chain_storage from consensus_contstants module to allow wallet to use it - minor simlification of wallet coin split logic - wallet and base node tests updated as needed
- adds consensus encoding for tari script and output features - igor: tx weight takes TariScript and OutputFeatures metadata into account as per tari-project#3368 - igor: weights updated as per tari-project#3368 - add `TransactionWeight` parameters for weatherwax and igor networks and set them in consensus constants - set max block weight for igor (+95% adjust to allow similar number of max inputs/outputs/kernels ~2Mb) - decouple chain_storage from consensus_contstants module to allow wallet to use it - minor simlification of wallet coin split logic - wallet and base node tests updated as needed
- adds consensus encoding for tari script and output features - igor: tx weight takes TariScript and OutputFeatures metadata into account as per tari-project#3368 - igor: weights updated as per tari-project#3368 - add `TransactionWeight` parameters for weatherwax and igor networks and set them in consensus constants - set max block weight for igor (+95% adjust to allow similar number of max inputs/outputs/kernels ~2Mb) - decouple chain_storage from consensus_contstants module to allow wallet to use it - minor simlification of wallet coin split logic - wallet and base node tests updated as needed
- adds consensus encoding for tari script and output features - igor: tx weight takes TariScript and OutputFeatures metadata into account as per tari-project#3368 - igor: weights updated as per tari-project#3368 - add `TransactionWeight` parameters for weatherwax and igor networks and set them in consensus constants - set max block weight for igor (+95% adjust to allow similar number of max inputs/outputs/kernels ~2Mb) - decouple chain_storage from consensus_contstants module to allow wallet to use it - minor simlification of wallet coin split logic - wallet and base node tests updated as needed
- adds consensus encoding for tari script and output features - igor: tx weight takes TariScript and OutputFeatures metadata into account as per tari-project#3368 - igor: weights updated as per tari-project#3368 - add `TransactionWeight` parameters for weatherwax and igor networks and set them in consensus constants - set max block weight for igor (+95% adjust to allow similar number of max inputs/outputs/kernels ~2Mb) - decouple chain_storage from consensus_contstants module to allow wallet to use it - minor simlification of wallet coin split logic - wallet and base node tests updated as needed
- adds consensus encoding for tari script and output features - igor: tx weight takes TariScript and OutputFeatures metadata into account as per tari-project#3368 - igor: weights updated as per tari-project#3368 - add `TransactionWeight` parameters for weatherwax and igor networks and set them in consensus constants - set max block weight for igor (+95% adjust to allow similar number of max inputs/outputs/kernels ~2Mb) - decouple chain_storage from consensus_contstants module to allow wallet to use it - minor simlification of wallet coin split logic - wallet and base node tests updated as needed
…or] (#3411) Description --- - adds consensus encoding for tari script and output features - igor: tx weight takes TariScript and OutputFeatures metadata into account as per #3368 - igor: weights updated as per #3368 - add `TransactionWeight` parameters for `weatherwax` and `igor` networks and set them in consensus constants - set max block weight for `igor` (+95% adjust to allow similar number of max inputs/outputs/kernels ~2Mb) - decouple chain_storage from `consensus_contstants` module to allow wallet to use it - wallet: minor simplification/dedup code of coin split logic - wallet: default fee per gram depending on network - wallet and base node tests updated as needed Motivation and Context --- See #3368 How Has This Been Tested? --- Existing tests updated, manually checked normal and one-sided transaction sending between console wallets/mobile on watherwax and igor
Description
Add
Block/Transaction Weight
section to Base node RFCRendered
This section probably raises a lot of questions but hopefully can trigger some discussion around the way we
weight block/transaction bodies and the way these weightings effect fees.
Some considerations:
input/output sets add a performance hit at tx/block validation timeMotivation and Context
Formally define block/transaction body weights taking into account TariScript and asset registration
How Has This Been Tested?
N/A docs