Skip to content
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

Hex encode the data field of the online transaction #2993

Open
wants to merge 1 commit into
base: albatross
Choose a base branch
from

Conversation

viquezclaudio
Copy link
Member

When a validator is reporting as online before the activation process, no genesis has been computed yet, so we use the default hash (0s) as part of the data field.

...

This fixes #___.

Pull request checklist

  • All tests pass. The project builds and runs.
  • I have resolved any merge conflicts.
  • I have resolved all clippy and rustfmt warnings.

@@ -72,7 +72,7 @@ pub fn generate_online_tx(validator: String) -> OutgoingTransaction {
to: Address::burn_address().to_user_friendly_address(),
value: 1, //Lunas
fee: 0,
data: Some("online".to_string()),
data: Some(Blake2bHash::default().to_hex()),
Copy link
Member

@sisou sisou Oct 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Default hash is 64 0s long. A lot of unnecessary data. You can get the utf8 bytes of a string with "online".bytes(), which you can then put as the data.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about keep using "online" but then realized that later in the process what we are doing is sending a hash (genesis hash), so I thought it was ok to send the default hash as an indication of "we have not computed a hash yet"
But since we are not doing anything useful with this field, I guess "online" or anything is fine

Copy link
Member

@sisou sisou Oct 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The 0-hash is actually still a valid genesis block hash :D

@hrxi
Copy link
Member

hrxi commented Oct 21, 2024

What's the motivation for changing it from "online"?

The data field of the online transactions needs to be a hex encoded string
@viquezclaudio viquezclaudio changed the title Use default hash when sending online transactions Hex encode the data field of the online transaction Oct 21, 2024
@@ -132,7 +132,7 @@ fn is_valid_ready_txn(
/// Checks if the provided transaction meets the criteria in order to be
/// considered a valid online-transaction
fn is_valid_online_txn(txn: &TransactionDetails, block_window: &Range<u32>) -> bool {
Some("online".to_string()) == txn.data
Some(hex::encode(String::from("online"))) == txn.data
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Um guessing this is optimized by the compiler for being static?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not, but it also doesn't really matter.

Suggested change
Some(hex::encode(String::from("online"))) == txn.data
Some(hex::encode("online")) == txn.data
Suggested change
Some(hex::encode(String::from("online"))) == txn.data
txn.data.as_ref().is_some_and(|hex| hex == "6f6e6c696e65") // "online"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants