common: add Jamtis base32 encoding #6

jeffro256 · 2023-09-10T23:01:47Z

see encoding scheme spec here: https://gist.github.com/tevador/50160d160d24cfc6c52ae02eb3d17024#35-base32-encoding

This PR is an alternative to #2. The motivation for this alternative was less code for reviewers (only about 60 real lines of code) and additional built-in functionality of the existing library cppcodec. The unit tests here include a sanity check for allowing for Jamtis address prefixes "xmra{1..9}{t,s,m}..." and a test to make sure that the added dependency doesn't change underneath our feet: base32.future_modification_protection.

src/common/base32_monero.h

rbrunner7 · 2023-09-14T16:54:27Z

Is this worth to review? Do you still intend to send this "into the ring" as possible alternative to the code that @DangerousFreedom1984 PRed? I am bit confused that you closed and then re-opened ....

jeffro256 · 2023-09-15T07:27:34Z

Yes, I did a hand-written version specifically because I wanted to see a mode where its easy to encode blocks of 5 bits at a time b/c the Jamtis body bit-size probably won't be an even multiple of 8, and the library code that @DangerousFreedom1984 adapted (or other libraries) did not seem like it would make that easy without large rewrites. There's also a no-allocate API provided here, which is quicker for fixed size fields (like Jamtis addresses), and this code also does mis-type and case normalization.

rbrunner7

I made a review and left some comments and questions after there were 2 votes in favor of this version versus 0 votes in favor of @DangerousFreedom1984 's Base32 PR.

I can't claim to understand every bit of calculation that is done here to code and encode, but well, the test cases show that the code works in principle, so I don't think that disqualifies my review.

tests/unit_tests/base32.cpp

src/common/base32.cpp

tests/unit_tests/base32.cpp

jeffro256 · 2023-09-18T03:57:05Z

Thanks for the review @rbrunner7

rbrunner7

Nice how many comments you added, thank you. Future people trying to find their way into the Monero cdebase might be very grateful :)

Looks good to me now.

src/common/base32.h

vtnerd · 2023-09-19T19:28:45Z

src/common/base32.h

+};
+
+// table of the base32 symbols, in Jamtis order
+extern const char JAMTIS_ALPHABET[32];


Do these tables really need to be exported? Can they be local to the cpp?

They are exported so they can be used by the base32 checksum PR #7 as default tables.

vtnerd · 2023-09-19T19:29:01Z

src/common/base32.h

+extern unsigned char JAMTIS_INVERTED_ALPHABET[256];
+
+// constants in the inverted table that signal an ascii code is invalid or ignoreable, respectively
+static constexpr const unsigned char BADC = 255;


Same with these constants, why export them?

They are exported so they can be used by the base32 checksum PR #7 as default tables.

src/common/base32.cpp

vtnerd · 2023-09-19T19:52:21Z

src/common/base32.h

+
+enum class Mode
+{
+    encoded_lossy, // when decoding, discard odd encoded LSB bits left at end of tail (default).


When is lossy useful? And how to select not lossy?

Mode::binary_lossy in useful for encoding exact blocks of 5 bits so that the encoded base32 string isn't as long. For example, Jamtis address body sizes will be an odd number of bits long, not divisible by 8. Thus, we can make the encoded string one byte shorter since there's leftover bits in the binary that we aren't using.

You select binary_lossy by passing it as the mode parameter in each function.

I thought it was expected when divisible by 5-bits that no additional values would be appended. I guess not. But then how does someone select lossless mode? There is no enum for it.

Although it isn't explicit, almost all base32 libraries take the "encoded lossy" approach which preserves every bit in the raw data and discards extraneous encoded string bits, since that's the expected behavior 90% of the time. That's the default behavior for this code too, but now you have the option.

But then how does someone select lossless mode?

You could make a lossless mode if and only if you forced the user to only encode raw data for which the byte length is divisible by 5, and decode encoded strings of which the length is divisible by 8.

It could be added, although I don't know when that would be useful.

vtnerd · 2023-09-19T21:44:45Z

src/common/base32.cpp

+            return static_cast<ssize_t>(Error::invalid_char);
+
+        // write symbol bits to current pointed-to byte
+        decoded_buf_out[byte_offset] |= v << 3 >> bit_offset;


I don't understand why the << 3 here. This should shift the first value by 3 (left), and I don't see how that could be accurate.

Since MSBs are encoded "before" LSBs, we shift the 5-bit alphabet index up 3 to align it with the first bit in the byte, the MSB, then we shift it according to the bit_offset.

This is a design choice where we could've encoded the LSBs before the MSBs, and not needed the << 3 but I like MSB->LSB type of encoding because it makes more sense for humans when you convert the raw data into a binary string.

That's also what most base32 libraries do anyways.

Yup, looked at the encoding algorithm to see what you did. I suppose it doesn't matter, as long as its consistent behavior.

jeffro256 · 2023-09-20T21:56:32Z

Thanks for the review @vtnerd, the newest commit should have all those changes you requested

see encoding scheme spec here: https://gist.github.com/tevador/50160d160d24cfc6c52ae02eb3d17024#35-base32-encoding 1. No-allocate API provided 2. "binary-lossy" mode, which lets us encrypt blocks of 5 bits at a time, useful for Jamtis addresses 3. Normalizes mis-typed characters and has case-insensitive decoding 4. Ignores hyphens when decoding 5. Error code handling

vtnerd reviewed Sep 10, 2023

View reviewed changes

src/common/base32_monero.h Outdated Show resolved Hide resolved

jeffro256 force-pushed the jamtis_base32_sm branch 2 times, most recently from 0c2a105 to 7a8baff Compare September 11, 2023 06:02

DangerousFreedom1984 mentioned this pull request Sep 13, 2023

base32 algorithm with a basic unit_test #2

Closed

jeffro256 closed this Sep 14, 2023

jeffro256 reopened this Sep 14, 2023

jeffro256 force-pushed the jamtis_base32_sm branch 2 times, most recently from e61709d to a175a69 Compare September 15, 2023 07:20

jeffro256 force-pushed the jamtis_base32_sm branch from a175a69 to f5eb9e8 Compare September 15, 2023 07:30

rbrunner7 mentioned this pull request Sep 15, 2023

Choosing between the 2 PRs implementing base32 support seraphis-migration/wallet3#60

Closed

rbrunner7 reviewed Sep 17, 2023

View reviewed changes

jeffro256 force-pushed the jamtis_base32_sm branch from f5eb9e8 to 1579bb3 Compare September 18, 2023 03:55

jeffro256 force-pushed the jamtis_base32_sm branch from 1579bb3 to a66ecf6 Compare September 18, 2023 05:00

rbrunner7 approved these changes Sep 18, 2023

View reviewed changes

vtnerd reviewed Sep 20, 2023

View reviewed changes

jeffro256 force-pushed the jamtis_base32_sm branch from a66ecf6 to f60bc53 Compare September 20, 2023 23:57

jeffro256 mentioned this pull request Sep 22, 2023

seraphis_impl: jamtis base32 checksums #7

Merged

jeffro256 force-pushed the jamtis_base32_sm branch from f60bc53 to ad1cb23 Compare September 22, 2023 16:05

vtnerd approved these changes Sep 25, 2023

View reviewed changes

rbrunner7 merged commit d7e89f7 into seraphis-migration:seraphis_wallet Sep 26, 2023

jeffro256 deleted the jamtis_base32_sm branch September 26, 2023 19:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

common: add Jamtis base32 encoding #6

common: add Jamtis base32 encoding #6

jeffro256 commented Sep 10, 2023 •

edited

Loading

rbrunner7 commented Sep 14, 2023

jeffro256 commented Sep 15, 2023 •

edited

Loading

rbrunner7 left a comment

jeffro256 commented Sep 18, 2023

rbrunner7 left a comment

vtnerd Sep 19, 2023

jeffro256 Sep 20, 2023

vtnerd Sep 19, 2023

jeffro256 Sep 20, 2023

vtnerd Sep 19, 2023

jeffro256 Sep 20, 2023

jeffro256 Sep 20, 2023

vtnerd Sep 20, 2023

jeffro256 Sep 20, 2023

jeffro256 Sep 20, 2023

jeffro256 Sep 20, 2023

vtnerd Sep 19, 2023

jeffro256 Sep 20, 2023

jeffro256 Sep 20, 2023

jeffro256 Sep 20, 2023

vtnerd Sep 20, 2023

jeffro256 commented Sep 20, 2023

common: add Jamtis base32 encoding #6

common: add Jamtis base32 encoding #6

Conversation

jeffro256 commented Sep 10, 2023 • edited Loading

rbrunner7 commented Sep 14, 2023

jeffro256 commented Sep 15, 2023 • edited Loading

rbrunner7 left a comment

Choose a reason for hiding this comment

jeffro256 commented Sep 18, 2023

rbrunner7 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

Choose a reason for hiding this comment

jeffro256 commented Sep 20, 2023

jeffro256 commented Sep 10, 2023 •

edited

Loading

jeffro256 commented Sep 15, 2023 •

edited

Loading