stacksgov / sips

Community-submitted Stacks Improvement Proposals (SIPs)
130 stars 80 forks source link

SIP-020 bitwise operators to Clarity 2 #106

Closed obycode closed 1 year ago

obycode commented 1 year ago

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: https://github.com/stacks-network/stacks-blockchain/pull/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.

obycode commented 1 year ago

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

LNow commented 1 year ago

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

wileyj commented 1 year ago

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

lgalabru commented 1 year ago

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):

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

obycode commented 1 year ago

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

Exactly.

LNow commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

radicleart commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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

jcnelson commented 1 year ago

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.

wileyj commented 1 year ago

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 commented 1 year ago

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

whoabuddy commented 1 year ago

@jcnelson yes! Catching up the official minutes today but the Governance CAB approves :+1:

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

obycode commented 1 year ago

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

jcnelson commented 1 year ago

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