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

add address_utils #17

Draft
wants to merge 28 commits into
base: seraphis_wallet
Choose a base branch
from

Conversation

DangerousFreedom1984
Copy link

Tool to convert JamtisDestination to/from an human-readable address 'xmra...'

UkoeHB and others added 20 commits February 8, 2024 14:53
…is_lib_hist_05_15_23 branch for commit history
make JamtisDestinationV1 serializable 

---------

Co-authored-by: DangerousFreedom <[email protected]>
* add operator== to JamtisPaymentProposals

---------

Co-authored-by: DangerousFreedom <[email protected]>\
* make JamtisPaymentProposal serializable

---------

Co-authored-by: DangerousFreedom <[email protected]>
…tProposals (UkoeHB#24)

* modify construct_tx_for_mock_ledger_v1 so it outputs the JamtisPaymentProposals

---------

Co-authored-by: DangerousFreedom <[email protected]>

// forward declarations

using namespace sp::jamtis;
Copy link

Choose a reason for hiding this comment

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

This is generally frowned upon, it defeats the purpose of namespaces entirely.

Choose a reason for hiding this comment

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

Yeah. Thanks. Removed.

std::string address = address_prefix;
address += static_cast<char>(address_version);
address += static_cast<char>(address_network);
address += base32::encode(serialized_address);
Copy link

Choose a reason for hiding this comment

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

It was decided to switch to base32 for Jamtis? Interesting, I don't recall a discussion about it though.

Copy link
Member

Choose a reason for hiding this comment

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

I think @tevador proposed a Base32 scheme together with a custom alphabet discussed here as integral part of his Jamtis spec, see here. There was some more discussion; e.g. @hyc asked why not Base64 here.

I had a chapter The Case for Base32 in my Reddit post about Jamtis addresses.

You could say that nobody opposed enough or came up with solid counter-arguments to topple Tevador's design decision, so we go along.

Choose a reason for hiding this comment

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

Well, I guess you are referring to why use base32 instead base58 for general purposes, right?
So I just ran the test below to compare the performance of base32 and base58 to encode/decode a string of length 1 to 500:

    std::string encoded_32, encoded_58;
    std::string decoded_32, decoded_58;
    std::string test;

    std::string my_str{"a"};

    std::vector<double> v_32;
    std::vector<double> v_58;

    test = my_str;
    for (size_t str_len = 0; str_len < 500; str_len++)
    {
        test += my_str;
        auto t58 = high_resolution_clock::now();
        for (size_t i = 0; i < 10000; i++)
        {
            encoded_58 = tools::base58::encode(test);
            tools::base58::decode(encoded_58, decoded_58);
            ASSERT_EQ(decoded_58, test);
        }
        auto t58_end = high_resolution_clock::now();

        auto t32 = high_resolution_clock::now();
        for (size_t i = 0; i < 10000; i++)
        {
            encoded_32 = base32::encode(test);
            decoded_32 = base32::decode(encoded_32);
            ASSERT_EQ(decoded_32, test);
        }
        auto t32_end = high_resolution_clock::now();


        duration<double, std::milli> ms_double32 = t32_end - t32;
        duration<double, std::milli> ms_double58 = t58_end - t58;

        v_32.emplace_back(ms_double32.count());
        v_58.emplace_back(ms_double58.count());
    }

And the result is the following:

image

Fortunately, base32 is about 30% faster. I believe we could also use that for the knowledge proofs or to store stuff in a human-readable format later. If speed matters.

Choose a reason for hiding this comment

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

Sorry, it was stupid, the result was obvious since my input was fixed. Anyway, if we come to a point where we should decide between base32 and base58 then I will do a proper benchmark. It was not my choice here.

Choose a reason for hiding this comment

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

Sorry. I completely misunderstood your comment about the encoding as I thought it would suffice to serialize the keys using the standard serialization method. But to be according the specs, we need to do some bit management to use all 910 bits sequentially.
*Now the specs are probably outdated but the encoding idea is the same (fill last bit of each X25519 point with last bits of address_tag to encode and do the opposite to decode).

address += base32::encode(serialized_address);

// 4. add checksum and get address_out
address_out = address + sp::jamtis::create_address_checksum(address);
Copy link

Choose a reason for hiding this comment

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

Why not return the std::string. There probably isn't any "memory re-use" with usage of the operator+ syntax.

Choose a reason for hiding this comment

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

Yeah, I think it doesnt matter. I added another function so the client decides :p

* derive view_balance from master key

Co-authored-by: UkoeHB <[email protected]>

---------

Co-authored-by: DangerousFreedom1984 <[email protected]>
Co-authored-by: UkoeHB <[email protected]>
@DangerousFreedom1984
Copy link
Author

Talking with @jeffro256, we realized that since the X25519 points occupies only 255 bits, there may be cases where two human-readable addresses point to the same keys. I will look that case in a couple days.

@DangerousFreedom1984 DangerousFreedom1984 marked this pull request as draft February 23, 2024 06:56
@jeffro256
Copy link

Related: the addresses can be made 1 byte shorter by packing in the X25519 pubkeys 255 bits at a time.

jeffro256 and others added 5 commits February 25, 2024 09:49
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
This code is efficient due to the use of lookup tables and the fact that no allocations are required.
…SERAPHIS]

This library has no dependency on wallet2.h and gives us a way forward to move away from `wallet2` in the (not-so-distant) future, while still supporting conversions of old wallet files.
This lib is also useful if you have an application where you want to extract information directly from the wallet file with or without having to setup accounts and devices. This is
now possible because I have split the wallet keys loading into two steps: `load_from_memory` and `setup_account_and_devices`. When one is loading a wallet keys file, the user of the API
can choose whether or not to contact external devices during this process with use of the flag `allow_external_devices_setup`.

Reorganized for `seraphis_lib`.
recovered_address = base32::decode(encoded_address, base32::Mode::binary_lossy);

// 3. shifts addr_tag 2 bits
recovered_address[113] >>= 2;
Copy link

@jeffro256 jeffro256 Mar 1, 2024

Choose a reason for hiding this comment

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

Check the string length before indexing, otherwise you can segfault, or worse

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.

8 participants