Initial tx_history #13

DangerousFreedom1984 · 2023-10-25T20:56:00Z

The idea is to collect transaction records (both for legacy and seraphis) authored by the wallet in order to provide the user the information he wants to look back. This component could be used to visualize detailed information of enotes or to generate knowledge_proofs for example.

This is the most basic component I could come up with. For sure new functionalities will be added in the future but for now it allows to show all the info from enotes and to generate/verify all knowledge proofs.

This PR contains the following:

Creation of Transaction History Component:

It allows to efficiently look at transaction records by searching the txid of sent txs.
The transaction records contains:
- the key_images of sent enotes (so the enote_records can be easily recovered from the enote_store).
- the JamtisPaymentProposals so the enote_ephemeral_private_key and destination addresses can be recovered.
Created (de)serialization functions to the components of the class so they can be imported/exported.

It creates/modifies the files:

encrypt_file.h (temporary)

Used to encrypt/decrypt and save/load SpTransactionStoreV1 to a file

serialization_types.h

Make/Recover serializable structs used in the wallet (transaction_history, knowledge_proofs, ...)

transaction_history.h

Contains SpTransactionStoreV1 which contains map by tx_id to TransactionRecordV1.
TransactionRecordV1 contains 1) key_images of spent enotes in a tx (so almost all the information about the tx can be retrieved looking at the corresponding enote at the EnoteStore) and 2) JamtisPaymentProposals (so all other information like destination and enote_ephemeral_privkey can be retrieved).
Functions to load (import) /save (export) TxHistory
Functions to add entries to TxHistory from a transaction
Functions to get enotes from a tx or last txs

transaction_utils.h

Useful functions for transactions (like going from JamtisDestination to the string address (xmra1...))

src/seraphis_mocks/mock_send_receive.h

Returns the JamtisPaymentProposals from the tx creation so it can be added to the transaction history
We dont care about modifying the seraphis_mock class as it is only used for unit_tests by now
The mock functions serve as an example to show how the corresponding real functions would look like

tests/unit_tests/sp_wallet_read_write.cpp

Unit_test to test the functions from the transaction_history.h

New tasks and open issues:

How to store wallet data on files? Is there a standard? The encrypt_file.h should be modified accordingly.

…is_lib_hist_05_15_23 branch for commit history

make JamtisDestinationV1 serializable --------- Co-authored-by: DangerousFreedom <monero-inflation-checker@protonmail.com>

Merge Seraphis library commits since June into the wallet branch

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

common: add Jamtis base32 encoding

fix base32 unit_test with single quote

This code is efficient due to the use of lookup tables and the fact that no allocations are required.

seraphis_impl: jamtis base32 checksums

* add operator== to JamtisPaymentProposals --------- Co-authored-by: DangerousFreedom <monero-inflation-checker@protonmail.com>\

* make JamtisPaymentProposal serializable --------- Co-authored-by: DangerousFreedom <monero-inflation-checker@protonmail.com>

…eHB#22)

…HB#19)

Seraphis wallet: Update to latest Seraphis library

rbrunner7

I made a first pass over the code. Mostly I did not yet look at too many coding details because I saw a number of quite fundamental issues with the PR as is. It may look quite different after addressing those, needed reviewing again afterwards.

rbrunner7 · 2023-10-27T13:00:25Z

src/seraphis_mocks/mock_send_receive.cpp

The changes in this file and the corresponding header file make this PR into a mixed Seraphis library + Seraphis wallet PR. I am still firmly convinced that making and merging such PRs directly to the Seraphis wallet, with nothing visible and nothing happening in the Seraphis library, is a very bad idea. I am afraid that this will get us into merge troubles due to conflicts in no time, and also has the disadvantage that @UkoeHB may miss what's happening and may not have a chance to also review and comment.

I am sorry, but unless the collective of all the other relevant working group member overrule me, I refuse to accept such "mixed" PRs. This would mean this has to go through the Seraphis library first and then find its way into the Seraphis wallet branch with a merge from there.

I agree with your statement

I refuse to accept such "mixed" PRs. This would mean this has to go through the Seraphis library first and then find its way into the Seraphis wallet branch with a merge from there.

for all seraphis_* folders except the seraphis_mocks.

Everything under the seraphis_mocks is temporary and used only for unit_tests. Koe already did a great work writing all these mock functions but they are not supposed to be permanent as far as I understand (though they are close to it). Otherwise they wouldnt be living under the mock folder. In this specific case, we are missing a function that creates a seraphis transaction and that would be part of the wallet. Since we dont have it yet (in the final form) we can do whatever we want with the mock files to test other components.

rbrunner7 · 2023-10-27T13:13:17Z

src/seraphis_mocks/mock_send_receive.cpp

+    SpTxSquashedV1 &tx_out,
+    std::vector<jamtis::JamtisPaymentProposalSelfSendV1> &selfsend_payment,
+    std::vector<jamtis::JamtisPaymentProposalV1> &normal_payment)
+{


If I checked correctly, the following happened here: There is a construct_tx_for_mock_ledger_v1 that is almost usable for your purposes, except that it does not give back those two kinds of payment proposals. Your reaction: You take that method, duplicate all 100 lines or so, add two output parameters and modify the duplicated code a bit to set those parameters.

Well, of course that technically works, but the resulting code duplication is a software engineering no-go. People looking at this in the future will never be fully sure that the two parameters are the only difference, because it's just too hard to compare the code of the two methods. And if something changes in transaction construction, you will have to modify forever two methods, and I guarantee you it does not take long until somebody modifies something and does not notice that there are two methods. Result: One broken and faulty transaction contruction method.

There are basically two sane and reasonable ways to do what is needed here:

A) You keep a single method, add the two parameters you need, and modify the places where the method is called in existing Seraphis library code accordingly.

B) You take the existing method, add the two parameters you need. Then you write a quite short and simple new method with the old signature that calls your modified method and just "throws away" the two vectors. This way you don't have to modify any code that calls the existing method.

Seeing that there are only few calls to the method, I would probably go way A).

I agree with your proposals to sane this issue. I have modified the mock and PRed into the seraphis_lib (just waiting to be accepted so the tests may fail for now). Another solution would be to move this function to a proto transactions_handling.cpp file under the seraphis_wallet for transaction handling like the work package 3 proposed here. Maybe this is the best solution to start moving things forward in this direction but that would also imply making the following structs (jamtis_mock_keys, legacy_mock_keys, MockLedgerContext) into something production ready. It is a task waiting to be picked up.

Just a general thought though, the seraphis_lib has not received such a detailed scrunity yet. A light piecewise PR as we are trying to do with the seraphis_wallet has not yet been done from the seraphis_lib to the monero code. I assume also that it would not be possible to break it file by file as they are very intertwined.

rbrunner7 · 2023-10-27T13:16:26Z

src/seraphis_mocks/mock_send_receive.cpp

+    const SpBinnedReferenceSetConfigV1 &bin_config,
+    const MockLedgerContext &ledger_context,
+    SpTxSquashedV1 &tx_out,
+    std::vector<jamtis::JamtisPaymentProposalSelfSendV1> &selfsend_payment,


Maybe it's a naming convention that is not very obvious, but if "it's several things", i.e. a vector, a list, or similar, you use plural. Just look up a few lines in the parameter list and note the plurals local_user_sp_keys and outlays.

So this parameter clearly needs to be selfsend_payments.

You are right. Thanks.

rbrunner7 · 2023-10-27T13:21:50Z

src/seraphis_wallet/encrypt_file.h

I would love to see this as a separate PR. With such, we could have a focussed discussion, maybe also involving other devs than just you and me, whether such a class is useful and a good idea, whether it uses a good approach, and so on. Now this feels out of place in your PR about tx history anyway.

I am not sure, maybe that's not a good idea, but how about breaking dependencies by making a version 0 PR for your tx history that does not yet actually write into files, so you don't have to wait until the separate PR with this class is merged? You can later add reading and writing to the tx history with a second PR.

I believe that an even higher level discussion would be: what files the wallet should create? So far I believe we decided for isolating the .keys from everything else (so it could be managed by key managers). But what is everything else? Should the TransactionHistory be saved together with the EnoteStore and all the other wallet parameters? Should they have different files?
For the moment I will comment the functions to save/load this content to/from a file and delete the encrypt_file.h but the serialization could still be done if we decide to save it with other things later.

rbrunner7 · 2023-10-27T13:29:58Z

src/seraphis_wallet/transaction_history.h

+};
+
+// struct to efficiently store/sort records
+struct SpTransactionStoreV1


If I come around I will write a separate issue with details about the question whether in the Seraphis wallet code we should follow the Seraphis library convention of versioning class names, like here with SpTransactionStoreV1 and not simply TransactionStore.

In short: I think in the Seraphis library this versioning is a good idea, because beside JamtisPaymentProposalV1 there may be soon JamtisPaymentProposalV2 and even JamtisPaymentProposalV3, and the library will have to support them all, concurrently. You won't be able to give up JamtisPaymentProposalV1. Indeed, if you could, the whole versioning would make no sense IMHO.

Is the same true about this transaction store? Is there a good probability that soon we will have a SpTransactionStoreV2 and that, even more importantly, wallet code will have to juggle with a SpTransactionStoreV1 and a SpTransactionStoreV2 object in parallel for some hard technical reason?

That somehow sounds pretty improbable to me. That leads me to a proposal to drop the "V1" here and go for SpTransactionStore.

But as I said, maybe that decision needs a broader discussion.

seraphis-migration/wallet3#62

rbrunner7 · 2023-10-27T13:37:56Z

src/seraphis_wallet/transaction_history.h

+
+class SpTransactionHistory
+{
+    SpTransactionStoreV1 m_sp_tx_store;


You have now two classes SpTransactionStoreV1 and SpTransactionHistory. What's are the reasons for two classes? Why not simply SpTransactionHistory?

I can't see any reasons right now, except the prediction that maybe soon a history will have transaction store V1 plus a transaction store V2 to deal with, and then you better have a class that can shield the rest of the code from the hassle that this will cause.

But well, if I am correct that fearing a transaction store V2 is unfounded, this falls away.

I have a humorous motto in the company I work: "Simple is simply simpler."

Regarding the TransactionStore and the TransactionHistory, I do believe it is better to keep it separated as it is now because it is cleaner to de/serialize and if things scale or other things are added to this component then it would be even cleaner and simpler to have it in a separated struct. I might be wrong though. Right now we just have one private variable which is the TransactionStore so it seems that I'm complicating things but in the future if more variables are added we might not want de/serialize them and it might be cleaner to have it this way.

rbrunner7 · 2023-10-27T13:39:55Z

src/seraphis_wallet/transaction_history.h

+     * param: selfsend_payments - selfsend payments
+     * param: normal_payments - normal payments
+     */
+    void add_single_tx_to_tx_history(const SpTxSquashedV1 &single_tx,


I think the name part to_tx_history is superfluous: We are here in the tx history, to what else would you want to add it? Plus, other methods do not have it either.

rbrunner7 · 2023-10-27T13:42:04Z

src/seraphis_wallet/transaction_utils.cpp

Again, I am not too happy to see this here, as part of this PR. It would be much nicer, if you ask me, to put this into its own PR. Focus and all. But it's small alright, so I wouldn't insist too hard.

Ok. If you are not happy with the location either I can put it in another place.

rbrunner7 · 2023-10-27T13:48:22Z

src/seraphis_wallet/transaction_utils.cpp

+using namespace sp::serialization;
+using namespace std;
+
+// Certainly there are faster ways to stringfy an enum


Well, your code looks ok to me. It's such a harmless case that will never grow over our heads in size, so why ready a large canon with some overly clever C++ wizzardry that half of use even don't understand?

But I am not sure it's a good idea to just return an empty string for anything else than V1. If we introduce Jamtis address version 2, we must catch this method as one that needs adjusting, and if you are tolerant here and just give back empty you mask the problem if we first forget to adjust.

With such translation methods, code written in my own "programming style" always throws an exception in the default case so that I notice as fast as possible if I extend an enum but forget to adjust some code accordingly.

Ok. Thanks.

DangerousFreedom1984 · 2023-10-28T22:58:57Z

First of all, thank you very much for taking the time to review it @rbrunner7 .

So, I updated this PR taking into consideration most of the comments and here are the new topics that some discussion would benefit this component:

Do we have the ability to add code into seraphis_mocks?
Would someone be interested in creating the transaction_handling.cpp file(s) to move most of the seraphis_mocks functions to the seraphis_wallet so we have something closer to being production ready for the txs creation?
Do we keep the same style of file creation as wallet2 (wallet and wallet.keys files)? If so, it means that at least the TransactionHistory and the EnoteStore should be serialized and stored in one file. Do we follow the same scheme for encrypting this file? What about the .keys file?

UkoeHB and others added 20 commits May 16, 2023 15:34

add key exchange round booster to multisig_account

e99b422

seraphis mega squash commit [initial commit - 05/15/2023]; see seraph…

0b10b72

…is_lib_hist_05_15_23 branch for commit history

async: explicit task constructors

dbd8c9c

rebase fixes

f3dda41

make JamtisDestinationV1 serializable (UkoeHB#11)

fb30680

make JamtisDestinationV1 serializable --------- Co-authored-by: DangerousFreedom <monero-inflation-checker@protonmail.com>

compile fix

7e9203f

clarify SpBasicEnoteRecordV1 docs; add async unit test for custom joins

ea9fa07

remove broken perf tests

eeca802

Merge pull request UkoeHB#9 from seraphis-migration/seraphis_lib

0a14382

Merge Seraphis library commits since June into the wallet branch

Merge pull request UkoeHB#6 from jeffro256/jamtis_base32_sm

d7e89f7

common: add Jamtis base32 encoding

fix base32 unit_test with single quote

774fdb9

Merge pull request UkoeHB#10 from DangerousFreedom1984/base32_quotes

7b3443e

fix base32 unit_test with single quote

seraphis_impl: jamtis base32 checksums

3149f3b

This code is efficient due to the use of lookup tables and the fact that no allocations are required.

Merge pull request UkoeHB#7 from jeffro256/jamtis_checksum_sm

9f0a03c

seraphis_impl: jamtis base32 checksums

add operator== to JamtisPaymentProposals (UkoeHB#18)

0a92514

* add operator== to JamtisPaymentProposals --------- Co-authored-by: DangerousFreedom <monero-inflation-checker@protonmail.com>\

make JamtisPaymentProposal serializable (UkoeHB#21)

8868b2d

* make JamtisPaymentProposal serializable --------- Co-authored-by: DangerousFreedom <monero-inflation-checker@protonmail.com>

util to convert legacy outputs to seraphis lib compatible enotes (Uko…

b6c52f0

…eHB#22)

container_helpers: is_sorted_and_unique 3x fewer comparison ops (Ukoe…

f5915c6

…HB#19)

Merge pull request UkoeHB#12 from seraphis-migration/seraphis_lib

702e5b1

Seraphis wallet: Update to latest Seraphis library

rbrunner7 reviewed Oct 27, 2023

View reviewed changes

rbrunner7 mentioned this pull request Oct 27, 2023

Do we use the Seraphis library versioning of class names for our wallet classes as well? seraphis-migration/wallet3#62

Open

DangerousFreedom1984 closed this Oct 28, 2023

DangerousFreedom1984 force-pushed the migration_tx_hist branch from 84538bd to 702e5b1 Compare October 28, 2023 22:27

initial tx_history

eafafc8

DangerousFreedom1984 reopened this Oct 28, 2023

remove encrypt_file

1669933

DangerousFreedom1984 marked this pull request as draft January 26, 2024 08:26

rbrunner7 force-pushed the seraphis_wallet branch from e2ca1aa to bf95191 Compare February 25, 2024 09:17

rbrunner7 force-pushed the seraphis_wallet branch from bf95191 to e651e0b Compare March 24, 2024 18:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial tx_history #13

Initial tx_history #13

DangerousFreedom1984 commented Oct 25, 2023

rbrunner7 left a comment

rbrunner7 Oct 27, 2023

DangerousFreedom1984 Oct 28, 2023

rbrunner7 Oct 27, 2023

DangerousFreedom1984 Oct 28, 2023

rbrunner7 Oct 27, 2023

DangerousFreedom1984 Oct 28, 2023

rbrunner7 Oct 27, 2023

DangerousFreedom1984 Oct 28, 2023

rbrunner7 Oct 27, 2023

rbrunner7 Oct 27, 2023

DangerousFreedom1984 Oct 28, 2023

rbrunner7 Oct 27, 2023

DangerousFreedom1984 Oct 28, 2023

rbrunner7 Oct 27, 2023

DangerousFreedom1984 Oct 28, 2023

rbrunner7 Oct 27, 2023

DangerousFreedom1984 Oct 28, 2023

rbrunner7 Oct 27, 2023

DangerousFreedom1984 Oct 28, 2023

DangerousFreedom1984 commented Oct 28, 2023

Initial tx_history #13

Are you sure you want to change the base?

Initial tx_history #13

Conversation

DangerousFreedom1984 commented Oct 25, 2023

This PR contains the following:

Creation of Transaction History Component:

It creates/modifies the files:

encrypt_file.h (temporary)

serialization_types.h

transaction_history.h

transaction_utils.h

src/seraphis_mocks/mock_send_receive.h

tests/unit_tests/sp_wallet_read_write.cpp

New tasks and open issues:

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DangerousFreedom1984 commented Oct 28, 2023