-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Add a transaction context function to get the raw transaction hash #13659
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #13659 +/- ##
=========================================
- Coverage 60.1% 60.0% -0.1%
=========================================
Files 856 856
Lines 210640 210692 +52
=========================================
- Hits 126615 126578 -37
- Misses 84025 84114 +89 ☔ View full report in Codecov by Sentry. |
@@ -182,6 +182,12 @@ module aptos_framework::transaction_context { | |||
payload.entry_function_payload | |||
} | |||
|
|||
/// Returns the hash of the current raw transaction. |
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.
Can we add documentation to explain the difference between this one and the other ones. May be say the other ones shouldn't be used?
This issue is stale because it has been open 45 days with no activity. Remove the |
This issue is stale because it has been open 45 days with no activity. Remove the |
any updates? |
I will add tests and more documentation this week. |
3f6dae2
to
71ec744
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.
Let's ship it
let raw_txn = RawTransactionWithData::new_fee_payer( | ||
txn.clone().into_raw_transaction(), | ||
secondary_signer_addresses.clone(), | ||
*fee_payer_address, |
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.
Let's make this ZERO instead...
We don't want to have two possible valid hash here.
cc @davidiw
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.
We could make the function return a vector of hash values to return both: [hash_for_0x0_fee_payer, hash_for_actual_fee_payer]
.
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.
what if it is not a fee payer txn? which two hashes would you return?
Also, I think users will be super confused.
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.
what if it is not a fee payer txn? which two hashes would you return?
Also, I think users will be super confused.
If it's not a fee-payer transaction, we could return a singleton vector with the hash of the raw transaction [hash_value].
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.
So it can be either 1 or 2?
Don't you think it is too confusing?
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.
So it can be either 1 or 2?
Yes, 2 return values for fee-payer transactions and 1 return value for other types of transactions.
Don't you think it is too confusing?
I think the more important question is whether it's necessary. The current authentication process utilizes both hash values with the actual fee payer and without it. Would there be any scenario where an AA user needs both hash values?
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.
Each txn must be either case but not both. Do you have any data on this, like how many people are using the each signing pre-image? We will note in the function that the hash is only for all zero if necessary.
I don't think there is a case that they need both. Do you have a use case in mind?
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.
The fee payer must sign the transaction with their own address included. The primary and secondary signers have two options: (1) Signing the transaction with the fee payer's address included, or (2) Signing the transaction with 0x0
as the fee payer address. I believe that option (2) is commonly used when the specific fee-payer account is determined later in the process. However, I don't have specific data to confirm this. Perhaps @davidiw, @gregnazario, or @movekevin may have more context on this.
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.
The second path is important.
The sender initially signs with 0x0, and then the fee payer signs with their address in that slot.
There are then 2 hashes, it would be the fee payer's responsibility to tell the user what the new hash is.
This is important to allow decoupling of knowing who the second signer is, because you don't really care as long as they pay
Though, this definitely points out a use case for a wait on sequence number function
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.
@lightmark, per request, I've updated the function to generate the hash value using 0x0 as the fee-payer address. If we need to generate hash values based on actual fee-payer addresses, we will need to introduce a separate API, as you suggested.
@@ -182,6 +182,13 @@ module aptos_framework::transaction_context { | |||
payload.entry_function_payload | |||
} | |||
|
|||
/// Returns the hash of the current raw transaction, which is used for transaction authentication. | |||
/// This function calls an internal native function to retrieve the raw transaction hash. | |||
public fun raw_txn_hash(): vector<u8> { |
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.
what will this be for non-user transactions? will it abort or returning something?
is there any uniqueness guarantees?
(irrespectively, function should be raw_txn_hash() -> get_raw_txn_hash() to follow the naming of other functions)
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.
what will this be for non-user transactions? will it abort or returning something?
It will abort when it's a non-user transaction and a user transaction context is unavailable.
aptos-core/aptos-move/framework/aptos-framework/sources/transaction_context.move
Line 273 in 71ec744
// expected to fail with the error code of `invalid_state(E_TRANSACTION_CONTEXT_NOT_AVAILABLE)` |
is there any uniqueness guarantees?
The returned hash values should differ if the raw transactions are different. However, it will remain the same across sessions (prologue, execution, and epilogue). In that sense, it's different from the get_transaction_hash
function.
(irrespectively, function should be raw_txn_hash() -> get_raw_txn_hash() to follow the naming of other functions)
The convention seems to avoid using the get_
prefix, except for get_transaction_hash
and get_script_hash
.
https://github.com/aptos-labs/aptos-core/blob/main/aptos-move/framework/aptos-framework/sources/transaction_context.move#L71-L183
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.
what's the hash from get_transaction_hash
?
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.
It's the hash of the SessionId
, which is an internal data structure within the Aptos VM.
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.
could we add a feature flag and reuse this function?
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.
get_transaction_hash is used extensively with the assumption it is a globally unique identifier to the session (across all transaction types)
new one isn't
so it will break anything that depends on it.
Naming here is very poor. Maybe we should deprecate this one (with #[deprecated]
), and add two new functions with clearer names, then compiler will warn people - and we can update in framework accordingly.
we can name them transaction_hash
and unique_session_hash
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.
Naming here is very poor. Maybe we should deprecate this one (with #[deprecated]), and add two new functions with clearer names, then compiler will warn people - and we can update in framework accordingly.
I've refactored the code by introducingunique_session_hash
and marking the original get_transaction_hash
function with the #[deprecated] attribute
. Even though the behavior of the original function remains unchanged, do we still need to feature-gate its deprecation?
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.
we can name them transaction_hash and unique_session_hash
How about raw_transaction_hash
? It's a bit more descriptive and less likely to cause confusion with other types of transaction hashes in the future.
3cbbd82
to
fa93aa3
Compare
9e9a62b
to
4f7ef2b
Compare
@@ -842,7 +842,7 @@ module aptos_framework::object { | |||
#[test(fx = @std)] | |||
fun test_correct_auid() { | |||
let auid1 = aptos_framework::transaction_context::generate_auid_address(); | |||
let bytes = aptos_framework::transaction_context::get_transaction_hash(); | |||
let bytes = aptos_framework::transaction_context::unique_session_hash(); |
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.
either you need a feature flag, or more simply - you can separate all usages of unique_session_hash() into separate PR that lands one release after that native function is introduced.
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.
@igor-aptos , since get_transaction_hash
is still available, should get_transaction_hash
still be used in this release instead of unique_session_hash
?
@@ -31,16 +31,19 @@ module aptos_framework::transaction_context { | |||
entry_function_payload: Option<EntryFunctionPayload>, | |||
} | |||
|
|||
/// Returns the transaction hash of the current transaction. | |||
native fun get_txn_hash(): vector<u8>; |
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.
you need to leave this for 1 release
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.
Got it. I added it back.
@@ -387,7 +406,10 @@ pub fn make_all( | |||
let natives = [ | |||
("get_script_hash", native_get_script_hash as RawSafeNative), | |||
("generate_unique_address", native_generate_unique_address), | |||
("get_txn_hash", native_get_txn_hash), | |||
( |
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.
just add both with same:
("get_txn_hash", native_unique_session_hash_internal),
("unique_session_hash_internal", native_unique_session_hash_internal),
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.
I added it back. Now, we have both.
@junkil-park can you resolve @igor-aptos 's comments? We need this for blind signing. |
A bit delayed due to a discussion about this issue on Slack. I will update shortly. |
4f7ef2b
to
b7576fc
Compare
a03f0fe
to
313ee22
Compare
assert!(features::transaction_context_hash_function_update_enabled(), error::invalid_state(ETRANSACTION_CONTEXT_HASH_FUNCTION_UPDATE_NOT_ENABLED)); | ||
raw_transaction_hash_internal() | ||
} | ||
native fun raw_transaction_hash_internal(): vector<u8>; |
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.
@igor-aptos , this is an entirely new native function (unlike get_txn_hash()
). Does this pattern seem right to you, having a public function wrapper and being protected by a feature gate?
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.
yes this is correct.
you can register raw_transaction_hash as native as well, so in next release you can remove raw_transaction_hash_internal completely
313ee22
to
df35fd7
Compare
assert!(features::transaction_context_hash_function_update_enabled(), error::invalid_state(ETRANSACTION_CONTEXT_HASH_FUNCTION_UPDATE_NOT_ENABLED)); | ||
raw_transaction_hash_internal() | ||
} | ||
native fun raw_transaction_hash_internal(): vector<u8>; |
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.
yes this is correct.
you can register raw_transaction_hash as native as well, so in next release you can remove raw_transaction_hash_internal completely
@@ -405,6 +424,10 @@ pub fn make_all( | |||
"multisig_payload_internal", | |||
native_multisig_payload_internal, | |||
), | |||
( |
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.
you need to add now:
("unique_session_hash", native_get_txn_hash),
so that second stage can be done
and you can rename native_get_txn_hash to native_unique_session_hash in rust for cleanliness, as this is just rust code
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.
Done renaming native_get_txn_hash
to native_unique_session_hash
,
and adding ("unique_session_hash", native_unique_session_hash),
and
("raw_transaction_hash", native_raw_transaction_hash_internal,),
@igor-aptos , by the way, we already have some existing native functions such as gas_payer_internal
that has its public wrapper gas_payer
.
Following the same pattern, we can register gas_payer
as native in this release, right? So, adding,
("gas_payer", native_gas_payer_internal),
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.
yup, if you want to clean things up and optimize out - we can do that.
if there is any chance we are going to need to revert a flag, we shouldn't remove the internal variant :)
df35fd7
to
dcfd72d
Compare
This commit adds a transaction context function to get the raw transaction hash.
dcfd72d
to
6b724bf
Compare
("get_txn_hash", native_unique_session_hash), | ||
("unique_session_hash", native_unique_session_hash), | ||
("sender", native_sender_internal), | ||
("sender_internal", native_sender_internal), | ||
("secondary_signers", native_secondary_signers_internal), | ||
( | ||
"secondary_signers_internal", | ||
native_secondary_signers_internal, | ||
), | ||
("gas_payer", native_gas_payer_internal), | ||
("gas_payer_internal", native_gas_payer_internal), | ||
("max_gas_amount", native_max_gas_amount_internal), | ||
("max_gas_amount_internal", native_max_gas_amount_internal), | ||
("gas_unit_price", native_gas_unit_price_internal), | ||
("gas_unit_price_internal", native_gas_unit_price_internal), | ||
("chain_id", native_chain_id_internal), | ||
("chain_id_internal", native_chain_id_internal), | ||
( | ||
"entry_function_payload", | ||
native_entry_function_payload_internal, | ||
), |
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.
great! :)
let raw_txn = RawTransactionWithData::new_fee_payer( | ||
txn.clone().into_raw_transaction(), | ||
secondary_signer_addresses.clone(), | ||
AccountAddress::ZERO, |
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.
Just curious about the reason behind using ZERO for fee-payer address. Is it because we want the execution of the transaction to be the same independent of which fee payer is paying for the transaction?
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 example, the sender and secondary signers can sign without specifying a particular fee-payer address, allowing the gas station to select an appropriate fee-payer account as needed.
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.
secondary signers can sign without specifying a particular fee-payer address,
Oh, I see. In practice, do secondary signers actually set the fee-payer address to ZERO? In the below sign_fee_payer
function, both the secondary signers and fee payer seem to be signing the same message. But then, in the verify function, the fee payer address is replaced with ZERO. I'm not sure why.
https://github.com/aptos-labs/aptos-core/blob/main/types/src/transaction/mod.rs#L298
https://github.com/aptos-labs/aptos-core/blob/main/types/src/transaction/authenticator.rs#L172
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.
The sender and secondary signers can sign with or without specifying a fee-payer address; both options are currently supported. However, the fee-payer is required to sign with its address included.
); | ||
raw_txn.hash().to_vec() | ||
}, | ||
_ => txn.raw_transaction_ref().hash().to_vec(), |
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.
SignedTransaction
actually already has the field committed_hash
that is computed during the previous stages of the execution pipeline. If we are okay with using hash(signed transaction)
here instead of hash(raw transaction)
, then we might save CPU cycles?
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.
We need the hash of the "raw transaction" that is used in the authentication process for signers.
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.
If we are okay with using hash(signed transaction)
, then I guess we may not need to construct RawTransactionWithData
as above (as TransactionAuthenticator will already have the info)?
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.
If we are okay with using hash(signed transaction)
In the current setting, I think it's not ok to use hash(signed transaction)
because TransactionAuthenticator
also contains signatures which should be excluded.
#[expected_failure(abort_code=196609, location = Self)] | ||
fun test_call_raw_txn_hash() { | ||
// expected to fail with the error code of `invalid_state(E_TRANSACTION_CONTEXT_NOT_AVAILABLE)` | ||
let _multisig = multisig_payload(); |
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.
The function seems pretty innocent :-)
Curious why it is supposed to fail.
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.
The unit test runs in an environment without any user transaction context. In that case, the design decision has been to abort instead of returning some default value.
/// In the case of a fee payer transaction, the hash value is generated using 0x0 as the fee payer address. | ||
public fun raw_transaction_hash(): vector<u8> { | ||
assert!(features::transaction_context_hash_function_update_enabled(), error::invalid_state(ETRANSACTION_CONTEXT_HASH_FUNCTION_UPDATE_NOT_ENABLED)); | ||
raw_transaction_hash_internal() |
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.
I'm actually not super familiar with why we do this. In general, why do we separate the native function into two functions with _internal
tag?
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 safety in release. It would've been ideal to just add public native fun raw_transaction_hash()
. However, it's turns out to be unsafe during the release process due to the timing not being perfectly aligned between the binary upgrade and the framework upgrade.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
✅ Forge suite
|
✅ Forge suite
|
✅ Forge suite
|
Description
This commit adds a transaction context function to get the raw transaction hash.
This resolves #14774.
Type of Change
Which Components or Systems Does This Change Impact?
How Has This Been Tested?
unit test and e2e test
Key Areas to Review
Checklist