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-0122? | Logical operations over BuiltinByteString #806

Merged
merged 32 commits into from
Jun 11, 2024

Conversation

kozross
Copy link
Contributor

@kozross kozross commented May 3, 2024

This CIP describes several operations over bits, including AND, OR and XOR, as well as indexing, setting, and clearing, operating on BuiltinByteString. Additionally, it provides a 'replication' operation to produce a BuiltinByteString of a given length full of identical bytes.

Human-readable form

@rphair rphair changed the title Logical operations over BuiltinByteString CIP-???? | Logical operations over BuiltinByteString May 3, 2024
@rphair rphair added the Category: Plutus Proposals belonging to the 'Plutus' category. label May 3, 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.

thanks @kozross, looks great - please keep us updated in PR comments about how the path-to-active progresses. 🙏

@michaelpj @zliu41 can you review this? I've added to the agenda for introduction at the next CIP Editor meeting: https://hackmd.io/@cip-editors/88

@Ryun1
Copy link
Collaborator

Ryun1 commented May 14, 2024

@michaelpj @zliu41 comment would be appreciated

Copy link
Contributor

@michaelpj michaelpj left a comment

Choose a reason for hiding this comment

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

Good work as usual! I'm basically happy with this.

CIP-XXX/CIP-XXX.md Outdated Show resolved Hide resolved
CIP-XXX/CIP-XXX.md Outdated Show resolved Hide resolved
CIP-XXX/CIP-XXX.md Outdated Show resolved Hide resolved
CIP-XXX/CIP-XXX.md Outdated Show resolved Hide resolved
CIP-XXX/CIP-XXX.md Outdated Show resolved Hide resolved
CIP-XXX/CIP-XXX.md Outdated Show resolved Hide resolved
CIP-XXX/CIP-XXX.md Outdated Show resolved Hide resolved
CIP-XXX/CIP-XXX.md Outdated Show resolved Hide resolved
CIP-XXX/CIP-XXX.md Outdated Show resolved Hide resolved
CIP-XXX/CIP-XXX.md Outdated Show resolved Hide resolved
@kozross kozross requested a review from michaelpj May 15, 2024 23:11
@rphair rphair changed the title CIP-???? | Logical operations over BuiltinByteString CIP-0122? | Logical operations over BuiltinByteString May 19, 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 excellent response to this at the CIP Editors' Meeting on Tuesday of this week. Editors (cc @Ryun1 @Crypto2099) will look to responses to points from @michaelpj's #806 (review) to see what to do next.

re: my #806 (review) about Path to Active, it looks good to me now... althrough if you think those bulleted items might progress intermittently I would suggest turning them into tickboxes.

CIP-XXX/CIP-XXX.md Outdated Show resolved Hide resolved
CIP-XXX/CIP-XXX.md Outdated Show resolved Hide resolved
Co-authored-by: Robert Phair <rphair@cosd.com>
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.

Looks good as of ef3ecb9 & will invite other editors to confirm.

@rphair rphair requested review from Ryun1 and Crypto2099 and removed request for michaelpj May 21, 2024 18:08
CIP-0122/CIP-0122.md Outdated Show resolved Hide resolved
CIP-0122/CIP-0122.md Outdated Show resolved Hide resolved
CIP-0122/CIP-0122.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.

I was looking at this while I was reviewing the implementation and added a few comments about fairly superficial things, most of which don't need any response. I didn't check things like the details of the semantics: I'll try to come back and review it more thoroughly later.

CIP-0122/CIP-0122.md Outdated Show resolved Hide resolved
CIP-0122/CIP-0122.md Outdated Show resolved Hide resolved
* Ignore the bytes of the longer argument whose indexes would not be valid for
the shorter argument, then perform the operation as if on matching-length
arguments. We call this _truncation semantics_.
* Fail with an error whenever argument lengths don't match. We call this
Copy link
Contributor

Choose a reason for hiding this comment

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

One advantage of failing is that you'd find out immediately if something's the wrong length. When padding/truncation happens automatically a single input of the wrong length in a long chain of calculations will propagate through to the very end and it might be difficult to work out where the problem originated. That's just a comment however: I'm not going to insist that the choice made here is the wrong one.


### Bit setting

`writeBits` in our description takes a change list argument, allowing
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is a good choice! I'd initially assumed that you'd have to do these things a bit at a time, and the resource bounds on on-chain scripts would really reduce the usefulness in that case. Allowing a list of updates is a neat solution to that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! I came at it from the point of memory use: if you're forced to copy something 10 times to change 10 bits, this is quite bad in a setting where you have limited resources.

## Abstract

We describe the semantics of a set of logical operations for Plutus
`BuiltinByteString`s. Specifically, we provide descriptions for:
Copy link
Contributor

Choose a reason for hiding this comment

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

A very pedantic comment: in Plutus Core the types are called bytestring, integer, and so on: see the Plutus Core specification. BuiltinByteString, BuiltinInteger etc are Haskell types in PlutusTx, which are not the same thing, and may not be meaningful to implementers of other languages targeting Plutus Core. I don't think it's necessary to change the notation though, since it's pretty clear what's going on (and I think the same notation was used in the integer/bytestring conversion CIP and nobody noticed). It might also be argued that the document should use the Plutus Core syntax ((con bytestring #123456) and so on) but that would require a lot of work and would just make the document much less readable. I think the mapping between the Haskell-type notation here and the lower-level syntax for Plutus Core should be pretty clear.

Copy link
Contributor

Choose a reason for hiding this comment

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

Now I'm wondering what "Plutus" really means in this document. Our names are a bit confusing and strictly there's no such thing as Plutus: there are only (typed and untyped) Plutus Core and the Haskell thing called PlutusTx. There's a suggestion that we should reduce confusion by reserving the name "Plutus" for the thing that runs on the chain (ie, untyped Plutus Core) and rename PlutusTx to something else. I don't know when (or if) that's likely to happen though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a good point more generally: some clarity in what terms we use in CIPs would be good to have, similar to how something like RFC-2119 specifies what certain words mean exactly and standardizes their use.

kozross and others added 2 commits June 6, 2024 07:13
Co-authored-by: Ryan <44342099+Ryun1@users.noreply.github.com>
@kozross kozross force-pushed the koz/logic-ops branch 5 times, most recently from 82f80e6 to 7597213 Compare June 5, 2024 23:08
@kozross kozross requested review from kwxm and Ryun1 June 5, 2024 23:54
@kozross
Copy link
Contributor Author

kozross commented Jun 5, 2024

@Ryun1 - I believe I have addressed your concerns now.

@kwxm - thanks for catching two quite confusing mistakes on my part. I have now fixed these, and hopefully clarified my intent with both.

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 we were ready to merge this at tonight's CIP meeting but first we need you to change the proposal document name from the current CIP-0122.md to README.md.

@kozross
Copy link
Contributor Author

kozross commented Jun 11, 2024

@rphair - done. Sorry for the confusion on my part.

@rphair rphair merged commit 2e740b2 into cardano-foundation:master Jun 11, 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.

5 participants