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

SIP-020 bitwise operators to Clarity 2 #106

Merged
merged 11 commits into from
Mar 21, 2023
Merged

Conversation

obycode
Copy link
Contributor

@obycode obycode commented Nov 12, 2022

This SIP proposes new bitwise operators to be added to Clarity2. It would need to activate with 2.1, as it is a consensus breaking change.

Reference implementation: stacks-network/stacks-core#3389 (in progress)

This change should not hold up code complete for 2.1. If the PR is not approved in time, then it should be saved for the next hard-fork.

In addition to sign off from the Technical CAB for this change, the SIP will also require approval from the Governance CAB to approve the activation strategy.

Copy link
Contributor

@jcnelson jcnelson left a comment

Choose a reason for hiding this comment

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

Mostly looks good. Just had a couple minor points. Once they're addressed, please feel free to transition this to Accepted status and call it SIP 020.

@obycode
Copy link
Contributor Author

obycode commented Nov 14, 2022

@LNow Members of the Technical CAB have requested your input on this SIP.

@LNow
Copy link

LNow commented Nov 14, 2022

@obycode I'm sorry, but I don't have time for this.

@wileyj
Copy link
Contributor

wileyj commented Nov 14, 2022

@lgalabru would you have some time to add some input here?

@lgalabru
Copy link
Contributor

lgalabru commented Nov 14, 2022

LGTM, but I'm a bit surprised / ambivalent on the shift behavior approach, and the "wrapping" strategy (<< 129 == << 1).
Could it become a foot gun for developers?
It does not look like there's a consensus between languages on how to address that, but many behavior can be observed in the nature (credit to @obycode for the quick research):

  • JS: wrapping
  • Swift: erroring
  • Rust: panicking

If we wanted to be consistent with the overflow/underflow runtime errors that we took for the math operators, we would throw a runtime error, no? And up to developers to add an explicit (mod 128) when it makes sense?

@jcnelson
Copy link
Contributor

If we wanted to be consistent with the overflow/underflow runtime errors that we took for the math operators, we would throw a runtime error, no?

I strongly believe that we do NOT want this. If the developer wants runtime errors for bit-shifting, then they can use *, /, and/or pow. Therefore, shifting ought NOT to throw errors so developers can choose which semantics they want.

LGTM, but I'm a bit surprised / ambivalent on the shift behavior approach, and the "wrapping" strategy (<< 129 == << 1).
Could it become a foot gun for developers?

Different microarchitectures handle it differently. I think it's undefined behavior in C, and it's definitely undefined in C++11. I'm fine with wrapping because it doesn't throw errors.

@obycode
Copy link
Contributor Author

obycode commented Nov 14, 2022

I'm comfortable with the masking behavior currently described in this SIP, or the behavior I originally had, where a shift amount > 128 will just clear all the bits and result in a 0 or -1, but I don't like the error behavior, because in typical uses of these bitwise operations, you'll be expecting to drop bits. As Jude said, if you want the error, you can use *, /, and pow.

@wileyj
Copy link
Contributor

wileyj commented Nov 14, 2022

If we wanted to be consistent with the overflow/underflow runtime errors that we took for the math operators, we would throw a runtime error, no?

I strongly believe that we do NOT want this. If the developer wants runtime errors for bit-shifting, then they can use *, /, and/or pow. Therefore, shifting ought NOT to throw errors so developers can choose which semantics they want.

LGTM, but I'm a bit surprised / ambivalent on the shift behavior approach, and the "wrapping" strategy (<< 129 == << 1).
Could it become a foot gun for developers?

Different microarchitectures handle it differently. I think it's undefined behavior in C, and it's definitely undefined in C++11. I'm fine with wrapping because it doesn't throw errors.

If this is something we don't want to include in this SIP...could it open the door to some unintended consequences (like the ones @lgalabru mentioned)?

@jcnelson
Copy link
Contributor

If this is something we don't want to include in this SIP...could it open the door to some unintended consequences (like the ones @lgalabru mentioned)?

Wrapping the shift operand is supported on all architectures, because architectures only start to exhibit undefined behavior when the shift amount exceeds the bit width of the integer being shifted. By explicitly masking the lower 128 bits of the shift argument, we avoid this altogether.

@jcnelson
Copy link
Contributor

I see the symbols have changed. Is that the feedback from the technical CAB?

@obycode
Copy link
Contributor Author

obycode commented Nov 15, 2022

I see the symbols have changed. Is that the feedback from the technical CAB?

Exactly.

@LNow
Copy link

LNow commented Nov 15, 2022

I'm finally off work, so here is my feedback:

Current description of each function might be difficult to understand by new developers who never had a chance to code in other language.
In my experience definitions provided by Microsoft are very clean and easy to digest. I think you could re-use them.
Here is example for << https://learn.microsoft.com/en-us/cpp/cpp/left-shift-and-right-shift-operators-input-and-output?view=msvc-170#left-shifts

Bitwise logic (and, or, xor) are easier to understand when you present table that shows two inputs and output, and right after that simple example with binary representation of each value used in such example.

AND &

Input A Input B Output
1 1 1
0 1 0
0 1 0
0 0 0
(& 17 30) ;; Returns 16
;; Input A (17):  0000000000010001
;; Input B (30):  0000000000011110
;; Output  (16):  0000000000010000 

Bit shifting examples should also have binary representation that shows exactly what is going on.

(<< 17 u2)  ;; Returns 68
;; Input (17):  0000000000010001
;; Output (68): 0000000001000100

And also it might be worth to mention (and show) that order in which we specify arguments in |, & and ^ doesn't matter.

(| 64 -32 -16) ;; Returns -16
(| 64 -16 -32) ;; Returns -16
(| -32 64 -16) ;; Returns -16
(| -32 -16 64) ;; Returns -16
(| -16 64 -32) ;; Returns -16
(| -16 -32 64) ;; Returns -16

It might be difficult to show in a readable way 128bit numbers, therefore examples in which you decide to show binary representation should have information that it has been simplified and shortened to X bits.

@LNow
Copy link

LNow commented Nov 16, 2022

If you want to activate this together with SIP-015, then I would advice to add a proper note to voting website (https://sip015.xyz)

cc: @whoabuddy @radicleart

@radicleart
Copy link
Contributor

@LNow i'll ask @jrmith to update the blog with the info that SIP-020 will activate with on SIP-015.

@radicleart
Copy link
Contributor

@LNow second thoughts maybe there is a better way to handle this - e.g. by running a second public vote in parallel to the main 2.1 upgrade. i'm raising this as a question.

@LNow
Copy link

LNow commented Nov 16, 2022

@radicleart this SIP was meant to be a "rider" on SIP-015, so I think its authors and core team didn't want it to be subject of any vote. However I believe they should be transparent and give users full picture of what they want to release with 2.1.

If they decide to put it under a separate vote - great. If not, then adding a note to description of 2.1 voting is the only reasonable way to handle it in a fair way.

@obycode
Copy link
Contributor Author

obycode commented Nov 16, 2022

The situation was that it was a last minute contribution from a community member and it seems like a non-controversial add on, and therefore not worth the effort of a separate vote, but we wanted the Governance CAB and Steering Committee to confirm that.

It definitely wouldn't hurt to add a note about it in the voting page.

@radicleart
Copy link
Contributor

@obycode happy to go with decision of governance cab / steering committee on this.

@jcnelson
Copy link
Contributor

The situation was that it was a last minute contribution from a community member and it seems like a non-controversial add on, and therefore not worth the effort of a separate vote, but we wanted the Governance CAB and Steering Committee to confirm that.

To add to this, there is no functional difference between a rider SIP and simply amending SIP-015 with the contents of this SIP. Either both SIP-015 and this SIP activate, or neither activate.

I was a proponent of creating a rider SIP to fold this change into the 2.1 upgrade. Given the feedback from the technical CAB on SIP-015 regarding their desire to see separate feature proposals written as separate SIPs in the future, I thought the act of creating a separate SIP for this would be better received than trying to update SIP-015.

@jcnelson jcnelson changed the title Add bitwise operators to Clarity 2 SIP-020 bitwise operators to Clarity 2 Nov 16, 2022
@jcnelson jcnelson added the Accepted SIP is in Accepted status label Nov 16, 2022
@wileyj
Copy link
Contributor

wileyj commented Nov 16, 2022

With the additional changes made over the past few days as recommended by the Technical CAB and in the comments here, this comment indicates approval of this SIP after a vote approving this proposal.

One comment of note regarding this proposal and the urgency to have it voted on for 2.1:

The added functionality is neither commonly requested by the community nor a blocker for contract developers. The original proposal has several red flags indicating the addition shouldn't be a priority, admitting that the lack of functionality is not a problem and that the solution just would be nice to have. In the discussion, Jude shares that when it comes to the new functions, you can implement them today with the existing operations. We should expect submitted SIPs to make the case that there is a substantial problem, with use cases demonstrating an actual need for a solution.

@jcnelson
Copy link
Contributor

@whoabuddy @obycode Has this been signed off on by both your CABs?

@whoabuddy
Copy link
Member

whoabuddy commented Nov 22, 2022

@jcnelson yes! Catching up the official minutes today but the Governance CAB approves 👍

Update: meeting minutes are in #107 and will be merged in once the CAB reviews one last time!

@obycode
Copy link
Contributor Author

obycode commented Nov 23, 2022

Yes, see @wileyj's message above. I sat this one out since I was the author and @wileyj took the lead.

Advance to Recommended
@jcnelson jcnelson added Recommended SIP is in Recommended status Activation-in-Progress SIP is in the process of activating and removed Accepted SIP is in Accepted status Recommended SIP is in Recommended status labels Nov 30, 2022
@jcnelson
Copy link
Contributor

Just an update on this -- we will merge this once 2.1 activates on mainnet.

@jcnelson jcnelson merged commit a9bbe19 into stacksgov:main Mar 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Activation-in-Progress SIP is in the process of activating
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants