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

seraphis_impl: jamtis base32 checksums #7

Merged

Conversation

jeffro256
Copy link

@jeffro256 jeffro256 commented Sep 11, 2023

This code is efficient due to 1) the lookup table API and 2) the fact that no allocations are required.

@jeffro256
Copy link
Author

This PR now depends on #6, but the actual PR code is now a lot simpler

@rbrunner7
Copy link
Member

When reviewing this code I had the following thoughts where the opinion of other Monero devs would interest me:

This code is now very C like. It does not use std::string, std::vector and other C++ plus STL things, but char *, length of buffers in extra size_t parameters, pointer arithmetic and such.

Compare this with another implementation of the Jamtis checksum algortithm here. That code looks quite a bit, well, cleaner, more modern, and better understandable to me.

Of course I am aware about the trade-offs: All that C++ plus STL "magic" with those std::string and std::vector objects does not come for free. It may well be that your code runs at double the speed of the other C++ style code and uses less memory as well.

But on the other hand, aren't things like buffer overflows and pointer arithmethic going astray somehow still very big problems in such C like code? Things that are almost impossible if you handle almost everything using STL classes?

And are we in a speed sensitive corner of our code here? I claim that our use case is not checksumming megabytes of base32 text where you would notice speed differences, but Jamtis addresses. And there not thousands of those, but maybe a handful in typical cases, or even a single one when validating a lone address on some form.

The base32 handling code is also written in the same style, and I think many of the things I mention here would apply there as well.

I bring this up not primarily because of this quite small code here. You can of course write this in C style and nothing really bad happens. But because we will write mountains of new code it may definitely matter in sum what kind of coding style is our "default".

@jeffro256 jeffro256 force-pushed the jamtis_checksum_sm branch 2 times, most recently from 1dfd2a4 to b592fec Compare September 22, 2023 15:27
@jeffro256
Copy link
Author

Things that are almost impossible if you handle almost everything using STL classes?

Nope, it just as easy to have memory errors using STL classes if you don't know what you're doing since those classes allow you to do direct pointer operations as well, it's just less explicit that you're doing them. For example, if I try have a string s, and try to get an iterator to the 5th character s.data() + 5, if I deference that iterator, I will segfault if the string's size isn't at least 5.

I agree though, that it would be useful to have STL overloads for ease-of-use. It's really use to add those in if you start with buffer APIs, but it's impossible to start with an STL API and then go to a buffer API without losing speed. I added std::string overloads in the last commit, what do you think?

@rbrunner7
Copy link
Member

For example, if I try have a string s, and try to get an iterator to the 5th character s.data() + 5

Well, why would you invoke data() in the first place if it was std::string, std::string everywhere :)

Overloads that take strings are certainly a net win and welcome if this stays "C style", which it probably will.

@rbrunner7
Copy link
Member

@jeffro256 : I am ready to merge, but I think in this form that's not a good idea, because the already merged Base32 stuff is included as well, and indeed GitHub right now tells me "This branch has conflicts that must be resolved" with "tests/unit_tests/base32.cpp" as conflicting file.

Not sure, but maybe best to re-PR this with only the pure checksum code?

@jeffro256
Copy link
Author

Yes, sorry I will rebase. Thanks for the reminder

This code is efficient due to the use of lookup tables and the fact that no allocations are required.
@rbrunner7 rbrunner7 merged commit 9f0a03c into seraphis-migration:seraphis_wallet Sep 29, 2023
15 of 18 checks passed
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.

3 participants