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-0058? | Bitwise primitives #283

Merged
merged 13 commits into from
Mar 1, 2023

Conversation

kozross
Copy link
Contributor

@kozross kozross commented Jun 23, 2022

This improves upon, and supercedes, the original. Specifically, the formatting has been designed to be clear and readable on Github, and concerns vis-a-vis semantics and wording raised here.

A few issues await @michaelpj's responses; in particular, the following questions need to be addressed:


See rendered markdown.

@kozross kozross mentioned this pull request Jun 24, 2022
58 tasks
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.

Looking good!

CIP-?/README.md Outdated
```
Performs a bitwise shift of the first argument by a number of bit positions
equal to the absolute value of the second argument, the direction of the shift
being indicated by the sign of the second argument.
Copy link
Contributor

Choose a reason for hiding this comment

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

This still doesn't say how the direction is indicated!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Clarified in the text, also for rotations.

CIP-?/README.md Outdated
of the represented byte sequence being treated in little-endian,
least-significant-bit-first encoding. For example, consider the byte sequence
$s = 110110011100000$; the `BuiltinByteString` literal corresponding to this would
be `"\217\224"`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Aha, are you referring to how the IsString instance works? I think it would be clearer to avoid that. For the purposes of this document BuiltinByteStrings are just bytestrings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rewritten to avoid completely.

CIP-?/README.md Outdated
all $i \in \\{ 0,1,\ldots,n \\}$, $s_i \in \\{ 0, 1 \\}$. A bit sequence
$s = s_n s_{n-1} \ldots s_0$ is a *byte sequence* if $n = 8k - 1$ for some
$k \in \mathbb{N}$. We denote the *empty bit sequence* (and, indeed, empty byte
sequence as well) by $\emptyset$.
Copy link
Contributor

Choose a reason for hiding this comment

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

How about this:
"...by $\emptyset$. We do not need to make any assumptions about how bytestrings are actually represented in Plutus, we give the semantics of the operations here, they can be implemented on whatever representation is actually used."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've gone for a slightly different rephrasing: the ordering is still specified, but a little more clearly.

CIP-?/README.md Outdated

We assume that `BuiltinByteString`s represent byte sequences, with the indexes
of the represented byte sequence being treated in little-endian,
least-significant-bit-first encoding. For example, consider the byte sequence
Copy link
Contributor

Choose a reason for hiding this comment

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

endianness is about integer encoding!

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 has now been moved to the relevant section.

CIP-?/README.md Outdated
We describe the translation of `BuiltinInteger` into `BuiltinByteString` which
is implemented as the `integerToByteString` primitive. Informally, we represent
`BuiltinInteger`s with the least significant bit at bit position $0$, using a
twos-complement representation. More precisely, let $i \in \mathbb{N}^{+}$. We
Copy link
Contributor

Choose a reason for hiding this comment

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

"This corresponds to a little-endian, least-significant-bit first encoding."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Amended.

CIP-?/README.md Outdated
We describe the semantics of `shiftByteString` and `rotateByteString`.
Informally, both of these are 'index modifiers' for bit sequences: given a
positive $i$, the index of a bit in $s$ 'increases' in the result; given a
negative $i$, the index of a bit in $s$ 'decreases' in the result. This can mean
Copy link
Contributor

Choose a reason for hiding this comment

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

Now you specify it. I just think this should be in the brief description too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now done.

@rphair
Copy link
Collaborator

rphair commented Jun 24, 2022

applying same labelling as held by deprecated version (#268 (comment))

@kozross kozross force-pushed the koz/bitwise branch 13 times, most recently from 3bdad5a to f8057e5 Compare September 14, 2022 23:26
CIP-?/README.md Outdated Show resolved Hide resolved
CIP-?/README.md Outdated Show resolved Hide resolved
CIP-?/README.md Outdated Show resolved Hide resolved
```

Convert a non-negative number to its bitwise representation, erroring if given a
negative number.
Copy link
Member

Choose a reason for hiding this comment

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

I reckon that the conversion uses big endianness. It could be nice to clarify it even though the interface described below seems pretty much agnostic to this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Pretty sure it's supposed to be little endian.

CIP-?/README.md Outdated Show resolved Hide resolved
We observe that `byteStringToInteger` 'undoes' `integerToByteString`:

```haskell
byteStringToInteger . integerToByteString = id
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
byteStringToInteger . integerToByteString = id
byteStringToInteger . integerToByteString = identity

(because readers may not be familiar with Haskell, let's not force the bad choices of the base Prelude on them).

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 assumes instead that they are familiar with one of the preludes that renames id to identity, which is even more unlikely than someone working on Cardano or Plutus not knowing Haskell.

Copy link
Member

Choose a reason for hiding this comment

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

No, it assumes they are familiar with word 'identity' which is pervasive in mathematics.

Copy link
Member

@KtorZ KtorZ Oct 25, 2022

Choose a reason for hiding this comment

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

I mean seriously, for anyone outside of the the tiny Haskell bubble, 'id' means 'identifier'.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Except the laws are specifically written in Haskell terms throughout. Furthermore, I really don't think anybody reading this won't be familiar with Haskell.

Copy link
Member

@KtorZ KtorZ Oct 25, 2022

Choose a reason for hiding this comment

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

This is merely a suggestion. Do as you please, but we aware that most people building on Cardano and participating in CIPs aren't actually that familiar with Haskell.

CIP-?/README.md Outdated Show resolved Hide resolved
CIP-?/README.md Outdated Show resolved Hide resolved
CIP-?/README.md Outdated Show resolved Hide resolved
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 this also mainly looks good to me... I'll keep an eye out for a commit making the changes in @KtorZ's last review (especially updating to Proposed status) and then approve & merge it as well.

@rphair
Copy link
Collaborator

rphair commented Oct 25, 2022

p.s. oops, I accidentally approved by ticking the wrong box... so we will wait for that commit before merging 🤪

@perturbing
Copy link
Contributor

For what it is worth, I made a similar implementation that this cip aims to achieve building on other builtins (this is thus not as efficient as the solution of this cip). Its still a work in progress.

I mostly followed the interface of the modules Crypto.Numbers (1) and Data.Bits (2).

Examples, of some basic functions

Prelude > import Plutus.Crypto.Number.Serialize as S
Prelude S > bs = S.i2ospOf_ 32 (2^255)
Prelude S > bs
"\128\NUL\NUL\NUL\NUL\NUL\NUL\NUL\NUL\NUL\NUL\NUL\NUL\NUL\NUL\NUL\NUL\NUL\NUL\NUL\NUL\NUL\NUL\NUL\NUL\NUL\NUL\NUL\NUL\NUL\NUL\NUL"
Prelude S > import Plutus.Data.Bits as B
Prelude S B > bs2 = B.setBit (S.i2ospOf_ 32 0) 255
Prelude S B > bs2
"\NUL\NUL\NUL\NUL\NUL\NUL\NUL\NUL\NUL\NUL\NUL\NUL\NUL\NUL\NUL\NUL\NUL\NUL\NUL\NUL\NUL\NUL\NUL\NUL\NUL\NUL\NUL\NUL\NUL\NUL\NUL\SOH"
Prelude S B > bs == B.reverseBS bs2
True

I am yet to implement the benchmarking of these functions (and optimize them). But if someone needs them, they can.

@rphair
Copy link
Collaborator

rphair commented Jan 11, 2023

@kozross marking "Waiting for author" because as far as I can tell this is ready to merge once the corrections from @KtorZ's last review are done (just to assign the official number & status & correct a half-dozen spelling mistakes).

@rphair rphair added the Waiting for Author Proposal showing lack of activity or participation from their authors. label Jan 11, 2023
Co-authored-by: Matthias Benkort <5680256+KtorZ@users.noreply.github.com>
@L-as
Copy link
Contributor

L-as commented Feb 22, 2023

@rphair I've committed all suggestions except the controversial one about id (which I don't agree with either). I think this can be merged now?

@rphair rphair merged commit 82e77b9 into cardano-foundation:master Mar 1, 2023
@L-as L-as deleted the koz/bitwise branch March 1, 2023 17:45
@rphair rphair removed the Waiting for Author Proposal showing lack of activity or participation from their authors. label Mar 2, 2023
@L-as
Copy link
Contributor

L-as commented Mar 9, 2023

PR: IntersectMBO/plutus#4733

@michaelpj
Copy link
Contributor

@L-as might be helpful to add an Implementation Plan and Implementors as per the new CIP-001, since I assume you're now the implementor?

@L-as
Copy link
Contributor

L-as commented Mar 10, 2023

Yes, indeed, I will do that.

@nielstron
Copy link
Contributor

There is also an implementation of bitmaps in the OpShin programming language and several builtins (such as chr, converting ints to corresponding unicode characters) that would benefit from these primitives

https://github.com/OpShin/opshin/blob/main/opshin/std/bitmap.py

Ryun1 pushed a commit to Ryun1/CIPs that referenced this pull request Nov 17, 2023
* Add CIP

* Notation fix-ups

* Fix heading levels on use cases

* Rewrite how shifts work

* Clarify findFirstSet description

* Clarify representation

* Clarifications

* Replace FFS with TZCNT and LZCNT, add finite field arithmetic example

* Rewrite

* Fix formatting

* Apply suggestions from code review

Co-authored-by: Matthias Benkort <5680256+KtorZ@users.noreply.github.com>

---------

Co-authored-by: goverthrow <goverthrow@protonmail.com>
Co-authored-by: Las Safin <me@las.rs>
Co-authored-by: Matthias Benkort <5680256+KtorZ@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants