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

fix: change truncate_from_bits to from_bits #5773

Merged

Conversation

SWvheerden
Copy link
Collaborator

Description

This removes all occurrences of truncate_from_bits to from_bits

Motivation and Context

truncate_from_bits will truncate all unknown bits and may cause bits to be interpreted as the wrong flag. This changes to the much more strict from_bits which forces peers to use the correct bits and only the correct bits.

Copy link
Member

@sdbondi sdbondi left a comment

Choose a reason for hiding this comment

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

utACK but since RpcFlags are not part of blockchain data this does limit some upgrade paths. Say we wanted to add a new optional flag to RPC that e.g. enables an optimisation for newer nodes, we could do that without breaking existing nodes. With this change we'll need to wait until sufficient have upgraded.

I can't think of a risk of truncating here given that this is a message flag for an ephemeral/non-persistent/not-hashed message.

@stringhandler
Copy link
Collaborator

stringhandler commented Sep 14, 2023

Yeah we may revert this when we add new flags, but I think for now let's err on the side of caution and use from_bits

@ghpbot-tari-project ghpbot-tari-project added P-acks_required Process - Requires more ACKs or utACKs P-reviews_required Process - Requires a review from a lead maintainer to be merged labels Sep 14, 2023
Copy link
Contributor

@hansieodendaal hansieodendaal left a comment

Choose a reason for hiding this comment

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

utACK

@ghpbot-tari-project ghpbot-tari-project removed the P-reviews_required Process - Requires a review from a lead maintainer to be merged label Sep 14, 2023
Copy link
Collaborator

@AaronFeickert AaronFeickert left a comment

Choose a reason for hiding this comment

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

utACK

@github-actions
Copy link

Test Results (CI)

1 214 tests   1 214 ✔️  25m 23s ⏱️
     39 suites         0 💤
       1 files           0

Results for commit 4644371.

@github-actions
Copy link

Test Results (Integration tests)

  2 files  11 suites   13m 11s ⏱️
28 tests 27 ✔️ 0 💤 1
29 runs  28 ✔️ 0 💤 1

For more details on these failures, see this check.

Results for commit 4644371.

@SWvheerden SWvheerden merged commit fb18078 into tari-project:development Sep 15, 2023
12 of 13 checks passed
@SWvheerden SWvheerden deleted the sw_remove_truncate_bites branch September 27, 2023 08:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P-acks_required Process - Requires more ACKs or utACKs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants