Tipped Transaction Type. #2930

kianenigma · 2019-06-23T14:12:05Z

Follow up on #2799 #2854.

~~Allows all three types of UnchckedExtrinsic to have a generic a tip enum field~~.
~~Optionally decoded to the tip upon receiving a tx. Default value if non-existent~~.
Visible to runtime/executive via a Tippable trait.

TODO:

Actually, use the tip value in validate_transaction to increase priority.
Check tip in the signature.
A dual node testnet fails now upon receiving a transaction in block_builder.rs due to an incorrect hash but all tests seem to pass which is kinda very bad. @tomusdrw @bkchr

WIP but pushed to get some initial feedback.

tomusdrw

I'd ditch the generic Balance from Tip if possible, and also please triple check that it's part of the signing payload.

core/sr-primitives/src/generic/tip.rs

tomusdrw · 2019-06-24T09:31:35Z

core/sr-primitives/src/generic/unchecked_mortal_compact_extrinsic.rs

 	/// The function that should be called.
 	pub function: Call,
+	/// The tip for this transaction
+	pub tip: Tip<Balance>,


You don't seem to modify fn check function and it's strictly required that tip is part of the payload we are signing. Otherwise you can change it arbitrarly and the signature would still be valid.

right -- added to the todo list up top.

cc @jacogr formats are changing

As Jaco noticed we could bump version here.

check is modified but I will keep the conversation open as a reminder that the extrinsic format has changed slightly.

node/executor/src/lib.rs

srml/executive/src/lib.rs

kianenigma · 2019-06-26T16:31:09Z

@tomusdrw two questions, one important and one more of a nit:

unimportant: Just some design decision:
- Tip can actually be a field in a Signature tuple or SignatureContent struct. I'd say it works better as a separate field (as it is now). I've also added tip to chekc() and will push it soon.
- Pass the tip also to CheckedExtrinsic (after check()) or not? I initially did this, then removed it. Functional-wise, it is not needed at all. Intuitive-wise, I think the clean approach is to have the tip a field in UncheckedExtrinsic (preferably non pub to depict that it is not checked with the sig yet) and then pass it to CheckedExtrinsic and implement Tippable for CheckedExtrinsic. Again, all of it seems to be optional and works fine as-is. (I will probably do this anyhow, my argument seems reasonable :p)
Important: The current situation forces all unsigned extrinsics to also have the tip field. Given your concerns of the tip to always be checked with the signature, Is that okay? The same new_unsigned() for UncheckedMortalCompactExtrinsic is also used somewhere related to inherents. I've so far ignored these details to get going faster but if you have some initial feedback, would be great.

tomusdrw · 2019-06-27T07:23:37Z

Tip can actually be a field in a Signature tuple or SignatureContent struct. I'd say it works better as a separate field (as it is now). I've also added tip to chekc() and will push it soon.

I don't think it semantically fits the signature. Also note that to produce the Signature you need the Tip to be part of signing payload, which complicates it even further.

Pass the tip also to CheckedExtrinsic (after check()) or not? I initially did this, then removed it. Functional-wise, it is not needed at all. Intuitive-wise, I think the clean approach is to have the tip a field in UncheckedExtrinsic (preferably non pub to depict that it is not checked with the sig yet) and then pass it to CheckedExtrinsic and implement Tippable for CheckedExtrinsic. Again, all of it seems to be optional and works fine as-is. (I will probably do this anyhow, my argument seems reasonable :p)

I think we should do that yes. Tip should be validated (aka checked) if it's valid and only then it can actually be used for anything.
I think we shouldn't really be using anything from Unchecked* structs, since you never know if they are sound before you check them.

Important: The current situation forces all unsigned extrinsics to also have the tip field. Given your concerns of the tip to always be checked with the signature, Is that okay?

So with the current values of Tip enum (None, Sender) the validation part should make sure that if Signature is missing the Tip can't be Sender. If we ever decide to introduce a different type, i.e. Tip::PayedBy(Signature) then we can have an unsigned transaction with Tip containing a signature and that's totally valid. The question is if we want to support something like that.

kianenigma · 2019-06-27T16:47:13Z

This can now be reviewed in more depth. There might still be some work on how the generic balance is handled but I consider that minor detail.

Once #2854 is stable and merged in with this I would like to add as many e2e tests as possible.

For now, my only manual, cumbersome testing method I had is broken due to this issue #2943. I think it is an important one actually.

node/runtime/src/lib.rs

core/sr-primitives/src/generic/tip.rs

…-tx-tip

core/sr-primitives/src/generic/unchecked_extrinsic.rs

core/sr-primitives/src/generic/unchecked_mortal_compact_extrinsic.rs

node-template/runtime/src/lib.rs

node/executor/src/lib.rs

srml/executive/src/lib.rs

srml/support/src/traits.rs

tomusdrw · 2019-06-28T14:37:57Z

srml/executive/src/lib.rs

+				// TODO: maximise (fee + tip) per weight unit here.
 				TransactionValidity::Valid {
-					priority: encoded_len as TransactionPriority,
+					priority: (encoded_len as TransactionPriority) + tip_value.saturated_into::<TransactionPriority>(),


I don't think this is correct. We seem to be adding a bytes length of the extrinsic and the value, but since MakePayment is generic, we don't really know how much each byte of encoded_len is worth, so it's hard to compare it directly with Balance.
I think MakePayment trait, should handle all this (maybe it requires a slight rename though):
fn make_payment(sender, encoded_len, tip) -> Result<Priority>;

So we make payment for both len and tip in one operation and we get the transaction priority back (or something that can be easily converted to priority).

srml/executive/src/lib.rs

kianenigma · 2019-07-10T09:45:57Z

@rphmeier the road map would be that this will be included in substrate and soonish we can try out replacing the type UncheckedExtrinic = <unchecked_mortal_compact_tipped_extrinsic> in polkadot's runtime directly.

core/sr-primitives/src/generic/unchecked_extrinsic.rs

core/sr-primitives/src/generic/unchecked_mortal_compact_extrinsic.rs

core/sr-primitives/src/generic/unchecked_mortal_compact_tipped_extrinsic.rs

tomusdrw · 2019-07-10T10:46:00Z

core/sr-primitives/src/generic/unchecked_mortal_compact_tipped_extrinsic.rs

+	Lookup, Checkable, Extrinsic, SaturatedConversion};
+use super::{CheckedExtrinsic, Era, Tip};
+
+const TRANSACTION_VERSION: u8 = 2;


Should we use different versioning than UncheckedMortalCompactExtrinsic? Say, start with 1000 or sth.

Not sure we can go that high :)

So the first byte is the encoded value, it is - signed flag & version, where signed flag is 0 for unsigned, or 0b10000000 for signed. So the lowest 7 bits indicate the version.

Tl;DR 127 max :)

Haha, right. I didn't notice it's only one byte :)

tomusdrw · 2019-07-10T10:46:36Z

core/sr-primitives/src/generic/unchecked_mortal_compact_tipped_extrinsic.rs

+		signed: Address,
+		signature: Signature,
+		era: Era,
+		tip: Option<Tip<Balance>>


should be required, since it's signed.

Suggested change

tip: Option<Tip<Balance>>

tip: Tip<Balance>,

In my head it makes sense; a signed can say my tip is none (which is practically the same as saying my tip is Sender(0)) but it is still fine as long as it is provided in the byte stream and it can be decoded (afaik None is 0 or 0 0 when encoded?).

ignore the above ^^ you're right.

node/executor/src/lib.rs

tomusdrw · 2019-07-10T11:02:02Z

srml/executive/src/lib.rs

 	Block: traits::Block<Header=System::Header, Hash=System::Hash>,
 	Context: Default,
 	Payment: MakePayment<System::AccountId>,
+	Balance: SimpleArithmetic + Copy,


Can't we use BalanceOf<Payment, System::AccountId> instead of additional generic type Balance?

Yes indeed we actually don't need this anymore; taking your previous comment balance is now an associated type of make_payment; hence not really needed up here.

gavofyork · 2019-07-10T18:06:46Z

is Tip<Balance> compact encoded?

gavofyork · 2019-07-10T18:07:30Z

core/sr-primitives/src/generic/checked_extrinsic.rs

+	/// Most often this is:
+	/// - `None` if the unchecked extrinsic does not have a tip OR it is unsigned.
+	/// - `Some` if the opposite.
+	pub tip: Option<Tip<Balance>>,


should just be an additional arg in signed

could be moved there but you mean probably in unchecked_mortal_compact_tipped.rs (god, hate this name to type it)? I will also update here but this is not really important anymore.

gavofyork · 2019-07-10T18:07:40Z

core/sr-primitives/src/generic/checked_extrinsic.rs

+pub struct CheckedExtrinsic<AccountId, Index, Call, Balance> {
 	/// Who this purports to be from and the number of extrinsics have come before
 	/// from the same signer, if anyone (note this is not a signature).
 	pub signed: Option<(AccountId, Index)>,


Suggested change

pub signed: Option<(AccountId, Index)>,

pub signed: Option<(AccountId, Index, Tip<Balance>)>,

it could theoretically be an option, but as long as it's compact encoded, then there's no point since zero compact encoded is a big as None encoded.

Just make sure Tip<Balance> implements Default. For non-tipping chains, Tip<Balance> can be ().

if it is part of the signature then it is only visible in signed txs which is the point of the current option. Hence, pub signed: Option<(AccountId, Index, Tip<Balance>)>, looks ok

gavofyork · 2019-07-10T18:10:24Z

core/sr-primitives/src/generic/tip.rs

+
+/// A placeholder tip type that is used to fulfill the generic requirements of `checked_extrinsic`
+/// when the underlying `unchecked_extrinsic` actually does not have a tip.
+pub type NoTipBalance = u32;


why not ()?

Could not have it meet the type requirements of the generic type (has to be SimpleArithmetic-- probably doable but needs a more handy rust dev with more time -- will try)

gavofyork · 2019-07-10T18:11:27Z

core/sr-primitives/src/generic/unchecked_mortal_compact_tipped_extrinsic.rs

+	pub function: Call,
+	/// The tip for this transaction. This is always `Some` for a signed transaction and always
+	/// `None` for unsigned.
+	pub tip: Option<Tip<Balance>>,


doesn't belong here since it's meaningless unless this is a (signed) transaction.

gavofyork · 2019-07-10T18:11:44Z

core/sr-primitives/src/generic/unchecked_mortal_compact_tipped_extrinsic.rs

+	/// The signature, address, number of extrinsics have come before from
+	/// the same signer and an era describing the longevity of this transaction,
+	/// if this is a signed extrinsic.
+	pub signature: Option<(Address, Signature, Compact<Index>, Era)>,


Suggested change

pub signature: Option<(Address, Signature, Compact<Index>, Era)>,

pub signature: Option<(Address, Signature, Compact<Index>, Era, Tip<Balance>)>,

gavofyork

Types could do with reworking.

kianenigma · 2019-07-11T14:04:09Z

will be closed by #3102

Kian Paimani and others added 7 commits June 19, 2019 16:54

Initial draft of tip enum

ab34400

Fix local tests

d36dc9a

Builds.

a18be8c

Fix test build.

55d39cf

Tratify tip.

a4e2816

Cleanup of checkedExt.

58ae208

More cleanup.

c7fe8bf

kianenigma added the A3-in_progress Pull request is in progress. No review needed at this stage. label Jun 23, 2019

Master.into()

e47bf96

devops-parity added the A4-gotissues label Jun 23, 2019

tomusdrw suggested changes Jun 24, 2019

View reviewed changes

kianenigma added 2 commits June 27, 2019 18:15

Checked Tip + other fixes.

78dde21

Line width

7326e5a

kianenigma requested a review from tomusdrw June 27, 2019 16:47

kianenigma added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. A4-gotissues labels Jun 27, 2019

rphmeier reviewed Jun 28, 2019

View reviewed changes

node/runtime/src/lib.rs Outdated Show resolved Hide resolved

4meta5 reviewed Jun 28, 2019

View reviewed changes

core/sr-primitives/src/generic/tip.rs Outdated Show resolved Hide resolved

Update core/sr-primitives/src/generic/tip.rs

a568a9b

devops-parity added the A4-gotissues label Jun 28, 2019

kianenigma added 4 commits June 28, 2019 16:15

Fix build.

b3f69cb

Merge branch 'master' of github.com:paritytech/substrate into kiz-tx-tip

557f6c5

Bump.

8e713aa

Merge branch 'kiz-tx-tip' of github.com:paritytech/substrate into kiz…

b110af6

…-tx-tip

tomusdrw reviewed Jun 28, 2019

View reviewed changes

jacogr mentioned this pull request Jun 29, 2019

Support optional extrinsic “tip” enum polkadot-js/api#1063

Closed

Make balance associated type of payment.

0d1de9d

kianenigma added A3-in_progress Pull request is in progress. No review needed at this stage. and removed A1-onice labels Jul 10, 2019

kianenigma added 4 commits July 10, 2019 10:25

Optionize tip.

6836f6b

Further cleanup.

7191a7d

Further cleanup.

0846065

Master.into()

12ecfcc

kianenigma requested review from rphmeier and tomusdrw July 10, 2019 09:44

Fix tests.

bf99e73

devops-parity added the B2-breaksapi label Jul 10, 2019

tomusdrw reviewed Jul 10, 2019

View reviewed changes

remove balance as generic.

4824776

kianenigma added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Jul 10, 2019

Update doc.

fb42e4e

kianenigma mentioned this pull request Jul 10, 2019

Operational Transactions #3092

Closed

kianenigma requested a review from tomusdrw July 10, 2019 17:40

gavofyork reviewed Jul 10, 2019

View reviewed changes

gavofyork suggested changes Jul 10, 2019

View reviewed changes

kianenigma closed this Jul 11, 2019

bkchr deleted the kiz-tx-tip branch July 11, 2019 16:17

sacha-l mentioned this pull request Jul 4, 2022

Advance FRAME: Signed Extensions Polkadot-Blockchain-Academy/pba-content#154

Merged

	pub signed: Option<(AccountId, Index)>,
	pub signed: Option<(AccountId, Index, Tip<Balance>)>,

	pub signature: Option<(Address, Signature, Compact<Index>, Era)>,
	pub signature: Option<(Address, Signature, Compact<Index>, Era, Tip<Balance>)>,

Uh oh!

Tipped Transaction Type. #2930

Tipped Transaction Type. #2930

Uh oh!

Conversation

kianenigma commented Jun 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tomusdrw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kianenigma commented Jun 26, 2019

Uh oh!

tomusdrw commented Jun 27, 2019

Uh oh!

kianenigma commented Jun 27, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kianenigma commented Jul 10, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jacogr Jul 10, 2019 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gavofyork commented Jul 10, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kianenigma commented Jun 23, 2019 •

edited

Loading

jacogr Jul 10, 2019 •

edited

Loading

gavofyork Jul 10, 2019 •

edited

Loading