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

CIP-0123? | Bitwise operations over BuiltinByteString #825

Merged
merged 14 commits into from
Jun 26, 2024

Conversation

kozross
Copy link
Contributor

@kozross kozross commented May 21, 2024

This adds bitwise operations to supplement the ones in CIP-122.

Human-readable version

@rphair rphair changed the title Bitwise operations CIP-???? | Bitwise operations May 22, 2024
@rphair rphair added the Category: Plutus Proposals belonging to the 'Plutus' category. label May 22, 2024
Copy link
Collaborator

@rphair rphair left a comment

Choose a reason for hiding this comment

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

@kozross exellent Rationale section & useful, precise Path to Active. Have put this on agenda to introduce at next CIP meeting (https://hackmd.io/@cip-editors/89) so it can be discussed along with these likely merging at that meeting:

@michaelpj @zliu41 @MicroProofs can you review this when you get a chance? p.s. you would all be welcome to attend that meeting to help with the Plutus review in general.

CIP-XXXX/CIP-XXXX.md Outdated Show resolved Hide resolved
CIP-XXXX/CIP-XXXX.md Outdated Show resolved Hide resolved
@rphair rphair requested review from Ryun1 and Crypto2099 May 22, 2024 12:06
@Crypto2099
Copy link
Collaborator

Since CIP-0122 is not yet merged in and "official" wouldn't it make sense to add this logic to that CIP before proceeding? I can see the value add of this but wonder what benefit we gain by breaking it up into two different CIPs rather than a single CIP to be implemented?

@rphair
Copy link
Collaborator

rphair commented May 28, 2024

@kozross I believe I can see the advantage to keeping this CIP separate from candidate CIP-0122 — based on a layman's reading of the Rationale for this CIP, which considers the possibility of implementing this CIP's primitives based on those of the earlier CIP (technically still a possibility) — but still we will have to consider @Crypto2099's #825 (comment) at tonight's meeting: so if you could answer that question in the next hour (or attend the meeting) it would be very welcome.

@rphair
Copy link
Collaborator

rphair commented May 28, 2024

p.s. @kozross at the CIP meeting just finished, we were wondering why these newer methods were not either added to CIP-0058 or submitted with an attempt to deprecate CIP-0058 (also asked in #806 (comment)).

@kozross
Copy link
Contributor Author

kozross commented May 28, 2024

p.s. @kozross at the CIP meeting just finished, we were wondering why these newer methods were not either added to CIP-0058 or submitted with an attempt to deprecate CIP-0058 (also asked in #806 (comment)).

These newer methods are modifications (and fairly minor ones) of CIP-58 equivalents. Deprecation of CIP-58 is already functionally the case anyway (as the Plutus team rejected the CIP-58 implementation): is there a more formal process I need to go through for actual deprecation of CIP-58?

@rphair
Copy link
Collaborator

rphair commented May 29, 2024

@kozross then I believe it would be best to submit a PR on CIP-0058 changing the header text from

Status: Proposed

to whatever best explains how it's "deprecated" or "superseded" (whichever language you think is best), e.g.

Status: Inactive (deprecated by CIP-0121 and CIP-0122)

according to CIP-0001 > Status: Inactive.

BTW when you submit that PR (we can also submit it for you, if you let us know what exact status to change to), I would also contribute some format cleanup before we put it to rest because it was missed in #389 for that effort.

@kozross
Copy link
Contributor Author

kozross commented May 29, 2024

I think 'superseded' is the right term here. Will send that PR ASAP.

CIP-XXXX/CIP-XXXX.md Outdated Show resolved Hide resolved
CIP-XXXX/CIP-XXXX.md Outdated Show resolved Hide resolved
Copy link
Contributor

@kwxm kwxm left a comment

Choose a reason for hiding this comment

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

This all looks perfectly fine: most of the design choices are kind of inevitable, so it's a lot more straightforward than the CIP for logical operations.


#### `findFirstSetBit`

Let $b$ refer to `findFirstSetBit`'s only argument, whose length in bytes is $n$,
Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose there's a slight danger of confusion here because we index bits in bytestrings from the right (which I agree is a good choice) but if you write down the bytestring in some form such as [0x1a, 0x35] or 0x1a35 or (con bytestring #1a35) this function will actually find what appears to be the last bit set. I think that's an unavoidable consequence of the way we write things down though (and the cultural fact that we read from left to right), and I can't think of a more sensible name. I guess we should just make it clear how it works in the documentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, 'first' here means 'based on our indexing scheme'. This whole left-versus-right situation is quite unfortunate, but as CIP-122 explains, unavoidable.

@kozross kozross force-pushed the koz/bitwise branch 4 times, most recently from 4b732df to 06f5fab Compare June 10, 2024 01:41
@kozross kozross requested a review from kwxm June 10, 2024 01:43
@rphair rphair changed the title CIP-???? | Bitwise operations CIP-0123? | Bitwise operations over BuiltinByteString Jun 11, 2024
Copy link
Collaborator

@rphair rphair left a comment

Choose a reason for hiding this comment

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

@kozross if these 3 proposals cannot live happily in an updated CIP-0058, the next best thing is for their numbers to be sequential. However we do need the title also to be made more specific since "Bitwise operations" is far too general for the content of this CIP.

Please also rename the containing directory to CIP-0123 and the proposal filename to README.md. FYI compliments on your work were made all around at the CIP meeting tonight.

CIP-XXXX/CIP-XXXX.md Outdated Show resolved Hide resolved
CIP-XXXX/CIP-XXXX.md Outdated Show resolved Hide resolved
@kozross
Copy link
Contributor Author

kozross commented Jun 11, 2024

@rphair - Done. Great to hear my work is up to standard!

Copy link
Collaborator

@rphair rphair left a comment

Choose a reason for hiding this comment

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

I think this fits in the same box as the recently merged #624 & #806 and it was again discussed (incidentally to the agenda) at tonight's CIP meeting, again without any reservations being expressed.

So @zliu41 @Ryun1 @Crypto2099 I'm going to reflect this in the Last Check tag, which should get it ready to consider merging at the next CIP meeting in 2 weeks' time. (cc @MicroProofs @kwxm)

@rphair rphair added the State: Last Check Review favourable with disputes resolved; staged for merging. label Jun 11, 2024
Copy link
Collaborator

@Crypto2099 Crypto2099 left a comment

Choose a reason for hiding this comment

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

Seems like this one has appropriately addressed all discussion points and meets the criteria that 121 and 122 also met to be merged in so I see no further blockers to allow this to progress.

@rphair rphair merged commit 33a0bd1 into cardano-foundation:master Jun 26, 2024
@rphair rphair removed the State: Last Check Review favourable with disputes resolved; staged for merging. label Sep 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Plutus Proposals belonging to the 'Plutus' category.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants