[PLT-8182] CIP-0087 support #5654

kozross · 2023-11-27T00:53:33Z

Pre-submit checklist:

Branch
- Tests are provided (if possible)
- Commit sequence broadly makes sense
- Key commits have useful messages
- Changelog fragments have been written (if appropriate)
- Relevant tickets are mentioned in commit messages
- Formatting, PNG optimization, etc. are updated
PR
- (For external contributions) Corresponding issue exists and is linked in the description
- Targeting master unless this is a cherry-pick backport
- Self-reviewed the diff
- Useful pull request description
- Reviewer requested

This adds support for the primitive operations described in CIP-0087. We also provide tests demonstrating both the properties, and examples, given for both primitive operations in the CIP-0087 document.

plutus-tx/src/PlutusTx/Builtins/Internal.hs

kwxm · 2023-12-04T23:34:44Z

@kozross Have you used some unusual setting in the tests? If I run cabal test untyped-plutus-core-test inourmain branch it behaves normally and there are some places where I see things like Running 38% with the number being updated in place. When I do the same in this branch I get hundreds of pages of output that look like

unning: 21%
unning: 22%
unning: 22%
unning: 22%
unning: 23%
unning: 24%
unning: 24%
unning: 24%
unning: 26%
unning: 26%
unning: 26%
unning: 27%
unning: 28%
unning: 28%
unning: 28%
unning: 29%
unning: 29%
unning: 30%
unning: 30%
unning: 31%
unning: 32%
unning: 32%
unning: 32%
unning: 34%
unning: 34%
unning: 34%
unning: 35%
unning: 36%

Presumably my terminal's failing to interpret some escape sequences properly, but I'm at a loss as to why that might be happening.

kozross · 2023-12-05T00:51:24Z

None of my settings are unusual, and I certainly didn't see anything strange in my terminal. The only thing I did was add more tests: I didn't change anything about how they would get run.

kwxm · 2023-12-05T02:39:16Z

Hmm. If I make the terminal more than 155 columns wide then it behaves normally, but any less than that and I get the weird behaviour I mentioned earlier. The final lines of output (on a wide terminal) look like this:

        ByteString -> Integer
          builtinByteStringToInteger b (builtinIntegerToByteString b d i) = i:                                                                 OK (24.10s)
              ✓ b2i_prop1 passed 8000 tests.
          builtinByteStringToInteger b (consByteString w8 emptyByteString) = w8:                                                               OK (22.97s)
              ✓ b2i_prop2 passed 8000 tests.
          CIP-0087 examples
            builtinByteStringToInteger False emptyByteString ==> failure:                                                                      OK
            builtinByteStringToInteger True emptyByteString ==> failure:                                                                       OK
            builtinByteStringToInteger False (consByteString 0x01 (consByteString 0x1 emptyByteString)) ==> 257:                               OK
            builtinByteStringToInteger True (consByteString 0x01 (consByteString 0x01 emptyByteString)) ==> 257:                               OK (0.01s)
            builtinByteStringToInteger True (consByteString 0x00 (consByteString 0x01 (consByteString 0x01 emptyByteString))) ==> 257:         OK
            builtinByteStringToInteger False (consByteString 0x00 (consByteString 0x01 (consByteString 0x01 emptyByteString))) ==> 65792:      OK
            builtinByteStringToInteger False (consByteString 0x01 (consByteString 0x01 (consByteString 0x00 emptyByteString) ==> 257:          OK
            builtinByteStringToInteger True (consByteString 0x01 (consByteString 0x01 (consByteString 0x00 emptyByteString) ==> 65792:         OK

I'm guessing that Tasty is aligning all of the output to accommodate these long lines at the end and for some reason if my terminal isn't wide enough then the output's incorrect. I think this must be some pecuilarity of the testing library and nothing to do with this PR.

Later: if I run our usual tests and narrow the window so that the output doesn't fit then I get similar results.

kwxm · 2023-12-05T02:47:10Z

^ See FAQ 2 here.

kwxm

This all looks pretty mergeable: there might be minor things that could do with a bit of tweaking, but the basic functionality all looks fine. I think we should wait for a review from at least one other person though: it'd be good to have someone looking at this who hasn't seen it before.

plutus-core/untyped-plutus-core/test/Evaluation/Builtins/Definition.hs

kwxm · 2023-12-05T14:01:15Z

plutus-tx/src/PlutusTx/Builtins.hs

+
+-- Conversions
+
+-- | Convert a 'BuiltinInteger' into a 'BuiltinByteString', as described in


For ease of reference it might be worth saying here that False = little endian and True = big endian, since that's quite hard to remember. I suppose there's nothing to stop us having separate builtinIntegerToByteStringLE and builtinIntegerToByteStringBE functions that fill in the endianness parameter for you, but that's probably out of scope for this PR and a user could easily define functions that use their preferred endianness anyway.

I'm happy to add this in, but the CIP actually spells out precisely what the arguments mean, so I'd just be repeating myself.

Yeah, but people don't follow links 🤷

plutus-core/untyped-plutus-core/test/Evaluation/Builtins/Conversion.hs

plutus-core/untyped-plutus-core/test/Evaluation/Builtins/Definition.hs

plutus-tx/src/PlutusTx/Builtins.hs

plutus-core/untyped-plutus-core/test/Evaluation/Builtins/Conversion.hs

michaelpj

Haven't reviewed the tests yet, but wanted to get something in! Generally things look pretty good.

plutus-core/plutus-core/src/PlutusCore/Builtin/Convert.hs

michaelpj · 2023-11-28T15:01:33Z

plutus-core/plutus-core/src/PlutusCore/Builtin/Convert.hs

+          pure EvaluationFailure
+        NotEnoughDigits -> do
+          emit "builtinIntegerToByteString: cannot represent Integer in given number of digits"
+          emit $ "Input: " <> (pack . show $ input)


michaelpj · 2023-11-28T15:05:15Z

plutus-core/plutus-core/src/PlutusCore/Builtin/Convert.hs

+and performance is thus critical, we choose to use this manually-specialized form
+for each combination of relevant arguments. While this is repetitive, and thus
+also somewhat error-prone, the performance penalty for not doing this is
+unacceptable.


Hmm. I am surprised you can't make this work. I'll try and take a think and see if I have any ideas.

I more-or-less listed all the approaches I took: out of all of them, the manual specialization method turned out to be noticeably better, especially for smaller inputs. This was quite surprising to me as well, but I can't really argue with the benchmark numbers.

michaelpj · 2023-11-28T15:09:33Z

plutus-core/plutus-core/src/PlutusCore/Builtin/Convert.hs

+free, as there is no data processing to be done: all we need to do is copy
+from one place to another, essentially.
+
+This technique only benefits us because counted arrays are cache-friendly: see


I assume this was borne out in benchmarks and isn't just a priori? (a priori argument is convincing though)

Yes, this definitely matters a lot. My original version of both functions didn't loop section at all, and I actually got more than a factor-of-8 speedup when I did the sectioning, as I reduced the number of integer operations by a factor of 8 (and they're linear), as well as the number of copy operations that'd be required (and they're also linear).

This is especially important, as I can't (easily) access the representation of Integers directly while being compatible with GHC 8.10. If and when 8.10 gets jettisoned, we can avoid all of this and just directly copy memory.

Since it sounds like you've already written the fast implementation, should we include it behind some CPP so it's ready to go later?

We discussed this with @kwxm, and decided against it, as it would mean that different nodes would run the same program with quite different costings. Due to the approach I'm taking here (to be 8.10 compatible), the costing is quadratic (Integer operation linearity forces our hand), but a direct copying method is linear.

Unless you mean just including it in the codebase behind an effective 'do not compile this'-CPP?

Sorry yes, I meant the latter. It can be some dead code that we can use later. It needs the CPP only because it presumably won't compile on 8.10.

It won't compile as-is on 9.2 either. Furthermore, the CIP has changed in a significant-enough way since that the implementation would need rethinking anyway. I don't think there's much reason to include my prior work as-currently-is: might be worth coming back to this question once 8.10 is gone.

plutus-core/plutus-core/src/PlutusCore/Builtin/Convert.hs

michaelpj · 2023-12-08T17:05:44Z

plutus-core/plutus-core/src/PlutusCore/Builtin/Convert.hs

+      | ix <= (limit - 7) =
+          let digitGroup = read64LE ix
+              newShift = shift + 64
+              newIx = ix + 8


shift is always ix * 8, right? I guess the correspondence isn't so simple in the BE case

Yes, but we adjust it in two different ways, due to loop sectioning.

michaelpj · 2023-12-08T17:06:31Z

plutus-core/plutus-core/src/PlutusCore/Builtin/Convert.hs

+          let digitGroup = read64LE ix
+              newShift = shift + 64
+              newIx = ix + 8
+           in if digitGroup == 0


does this optimization matter in practice? I wouldn't be shocked if it was even faster to avoid the test and branch and just unconditionally do the shift and addition

I did some benchmarks to verify this. In short, it doesn't really matter much: 10% on microbenchmarks like this is almost indistinguishable from noise, and furthermore, there's no evidence of linear growth of speedup or slowdown, which is what we would expect to see if the branching was costing us.

Just to be absolutely sure, I wrote another benchmark against the best possible case for this optimization: where you have a lot of significant zeroes sandwiched between two significant non-zeroes. However, even there, there's no major conclusive advantage. Given that less code is better than more code, I'll eliminate this branch everywhere it appears.

plutus-core/plutus-core/src/PlutusCore/Default/Builtins.hs

michaelpj · 2023-12-08T17:11:15Z

plutus-tx/src/PlutusTx/Builtins.hs

+
+-- Conversions
+
+-- | Convert a 'BuiltinInteger' into a 'BuiltinByteString', as described in


Yeah, but people don't follow links 🤷

plutus-core/plutus-core/src/PlutusCore/Builtin/Convert.hs

michaelpj · 2023-12-11T10:33:52Z

I have an ugly suggestion which I think you will both hate.

At the moment the padding argument is a little awkward:

It can be very big
It's awkward to cost because the cost depends on the value not the size

What if... the padding argument was instead another integer, and the meaning was to pad the result to be at least as wide as that integer. That would I think simplify the problems (it could still be too big, but in a more normal way). But it would be more complicated to implement, and probably bad to use (although the constant-folder should prevent you from paying to compute 2^64).

kozross · 2023-12-11T17:09:22Z

@michaelpj - this is both awkward to implement and awkward to use. On the implementation side, I would now have to tear down two Integers, in parallel, which effectively doubles the work. On the user side, you now have to figure out a constant of exactly the right byte width, plus the semantics are quite confusing.

If our goal is to avoid wraparound, we could just fail out on negative arguments altogether. Large positive Integers truncate, and those would definitely be ruled out by costing. I initially didn't do this (mostly to avoid having yet more error conditions), but the more I look at it, the more it seems to make sense to do this.

michaelpj · 2023-12-11T17:20:04Z

If our goal is to avoid wraparound, we could just fail out on negative arguments altogether. Large positive Integers truncate, and those would definitely be ruled out by costing.

I'm happy with rejecting negative arguments, and maybe also large positive arguments that don't fit into an Int.

kozross · 2023-12-11T22:13:27Z

If we go with rejecting arguments outside of the non-negative limits of Int, that guarantees us 2^29-1. That's surely big enough for anyone: a ByteString of that size is half a gigabyte, which is well in excess of anything that could possibly fit.

Given this, I feel it's the right thing to do. I will amend the CIP with this in mind, and update the PR accordingly.

michaelpj

Good stuff, I think we should merge this soon. Just one question about a magic number.

michaelpj · 2023-12-14T09:44:22Z

plutus-core/plutus-core/src/PlutusCore/Builtin/Convert.hs

-  | paddingArg > 10240 = do
+integerToByteStringWrapper endiannessArg lengthArg input
+  -- Check that we are within the Int range on the non-negative side.
+  | lengthArg < 0 || lengthArg >= 536870912 = do


Can we compute this? I'm not actually sure where this number comes from.

2^29. It's the positive bound on Int guaranteed by GHC.

drospa · 2023-12-29T13:35:22Z

@kozross There is a wrong CIP reference. It shall be CIP-0058 . That the one that introduces the BuiltinByteString primitives

kozross · 2023-12-31T18:27:16Z

@drospa - if you read the CIP I reference, you will find that it specifically explains why 0058's solution is not a good solution. Referencing it here makes no sense.

michaelpj · 2024-01-02T15:03:04Z

kwxm · 2024-01-02T23:22:39Z

@drospa - if you read the CIP I reference, you will find that it specifically explains why 0058's solution is not a good solution. Referencing it here makes no sense.

I think that @drospa is suggesting that the CIP (CIP-0087) mentioned in the PR title is the wrong one. I'm a bit confused about this myself. As far as I can see there is no CIP-0087, and CIP-0058 is the relevant one. Is that correct?

The implementation here does differ from what CIP-0058 says, so we should presumably update that at some point to reflect what's actually the case (and we should seek "official" approval for the CIP as well, but I'm not how to initiate that the process).

kozross · 2024-01-03T18:56:25Z

@kwxm - this is a bit of a sync issue. Originally, 0087 was the number I gave to the CIP PR that this work is based on. I was told later that CIP numbers are assigned post-merge, rather than on-PR: thus, I had to rename it to CIP-XXX temporarily. However, the response to the CIP PR took a while, and therefore, I didn't update the title of this PR.

It shouldn't matter much anyway, as all references to the CIP in the documentation aim at the correct document, though they will need updating once the CIP PR merges.

kozross added 11 commits November 22, 2023 13:51

Move conversion code into Plutus Core

0dc2637

Merge branch 'master' into koz/cip-0087

94757e7

Documentation and notes on implementations

77b606f

Wrap implementations into builtins

a8798ad

Merge branch 'master' into koz/cip-0087

1b942c5

Properties as per CIP-0087

ea205dd

CIP-0087 examples as tests

3bc9afa

Merge branch 'master' into koz/cip-0087

1376602

Add new builtins to PlutusTx

747fe3b

Document fromIntegral usage as a note

5a6c93a

Changelogs for CIP-0087 primitives

d3e1a49

catch-21 reviewed Nov 27, 2023

View reviewed changes

plutus-tx/src/PlutusTx/Builtins/Internal.hs Show resolved Hide resolved

Merge branch 'master' into koz/cip-0087

4e1ef6b

kozross force-pushed the koz/cip-0087 branch 4 times, most recently from 19630d5 to 15a5ceb Compare November 28, 2023 20:34

Ensure conversions don't break on too-large arguments

1b6f849

kozross force-pushed the koz/cip-0087 branch from 15a5ceb to 1b6f849 Compare November 28, 2023 20:53

kozross added 2 commits November 29, 2023 10:26

Ensure that conversions are available in V3

e9be20a

Merge branch 'master' into koz/cip-0087

d9ff42f

Merge branch 'master' into koz/cip-0087

2016ad1

kwxm approved these changes Dec 5, 2023

View reviewed changes

kwxm requested review from michaelpj and zliu41 December 5, 2023 20:27

Remove unnecessary pragmata on tests

fa90942

kozross added 6 commits December 7, 2023 13:20

Fix overly-long test names, clarify test meaning in comments

db91938

CIP link consistency

ab94f21

Merge branch 'master' into koz/cip-0087

d0594ca

Re-order integerToByteString arguments, avoid unnecessary padding

0b8aad3

Better documentation for implementations

dfd2fa3

Correct properties for ByteStringToInteger

d4bc659

michaelpj reviewed Dec 8, 2023

View reviewed changes

michaelpj reviewed Dec 11, 2023

View reviewed changes

plutus-core/plutus-core/src/PlutusCore/Builtin/Convert.hs Outdated Show resolved Hide resolved

kozross added 2 commits December 13, 2023 09:16

Merge branch 'master' into koz/cip-0087

77f026a

Address feedback

f2a7c26

kozross force-pushed the koz/cip-0087 branch from b8a15a6 to f2a7c26 Compare December 14, 2023 00:39

michaelpj approved these changes Dec 14, 2023

View reviewed changes

drospa changed the title ~~CIP-0087 support~~ [PLT-8182] CIP-0087 support Dec 27, 2023

michaelpj merged commit f734591 into IntersectMBO:master Jan 2, 2024
5 checks passed

kwxm mentioned this pull request Jan 19, 2024

Bitwise costing (PLT-8790) #5733

Merged

kwxm mentioned this pull request Jun 9, 2024

Bitwise operations #4733

Closed

58 tasks


		-- Conversions

		-- \| Convert a 'BuiltinInteger' into a 'BuiltinByteString', as described in

[PLT-8182] CIP-0087 support #5654

[PLT-8182] CIP-0087 support #5654

Conversation

kozross commented Nov 27, 2023

kwxm commented Dec 4, 2023

kozross commented Dec 5, 2023 • edited Loading

kwxm commented Dec 5, 2023 • edited Loading

kwxm commented Dec 5, 2023

kwxm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelpj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelpj commented Dec 11, 2023

kozross commented Dec 11, 2023

michaelpj commented Dec 11, 2023

kozross commented Dec 11, 2023 • edited Loading

michaelpj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drospa commented Dec 29, 2023

kozross commented Dec 31, 2023

michaelpj commented Jan 2, 2024

kwxm commented Jan 2, 2024

kozross commented Jan 3, 2024

kozross commented Dec 5, 2023 •

edited

Loading

kwxm commented Dec 5, 2023 •

edited

Loading

kozross commented Dec 11, 2023 •

edited

Loading