seraphis-migration / wallet3

Info and discussions about a hypothetical full 'wallet2' rewrite from scratch
MIT License
13 stars 0 forks source link

'wallet2' interface documentation: Query info about enotes, "payments" and transactions #49

Open rbrunner7 opened 1 year ago

rbrunner7 commented 1 year ago

The following text documents a small but very important subset of the public wallet2 interface: all methods to query info about enotes, "payments" and transactions, incoming and outgoing, confirmed and unconfirmed, together with the structs used to return the info. (The complete header file is here.)

The idea is supporting work on the top-level public interface of the new Seraphis wallet, sometimes called wallet3, its API so to say. By showing as clearly as possible what exists now it hopefully makes it easier to define the corresponding methods of the new wallet, and to define them in a better way, especially regarding terminology. It may help to decide how to store the info wallet-internally as well.

I also compared this most basic API with 4 other places where you can query the info:

Ideally there would be much more simularity and regularity than today between the lowest-level Seraphis wallet API, the RPC wallet interface, and "higher" APIs that make it easy to program wallet apps like the wallet2 API or the Monero C++ library.

Confirmed enotes

void get_transfers(
  wallet2::transfer_container& incoming_transfers) const;

typedef std::vector<transfer_details> transfer_container;

struct transfer_details
{
  uint64_t m_block_height;
  cryptonote::transaction_prefix m_tx;
  crypto::hash m_txid;
  uint64_t m_internal_output_index;
  uint64_t m_global_output_index;
  bool m_spent;
  bool m_frozen;
  uint64_t m_spent_height;
  crypto::key_image m_key_image; //TODO: key_image stored twice :(
  rct::key m_mask;
  uint64_t m_amount;
  bool m_rct;
  bool m_key_image_known;
  bool m_key_image_request; // view wallets: we want to request it; cold wallets: it was requested
  uint64_t m_pk_index;
  cryptonote::subaddress_index m_subaddr_index;
  bool m_key_image_partial;
  std::vector<rct::key> m_multisig_k;
  std::vector<multisig_info> m_multisig_info; // one per other participant
  std::vector<std::pair<uint64_t, crypto::hash>> m_uses;
}

This gets the list of all confirmed enotes. transfer_details must describe a single enote e.g. because there is only a single amount and a single destination m_subaddr_index. The term transfer is used for several different things, but in wallet2 in combination with "incoming" it is pretty unambiguous. This call is the basis for the CLI wallet command incoming_transfers and RPC wallet COMMAND_RPC_INCOMING_TRANSFERS.

There can be several enotes with the same incoming transaction id txid because several enotes for us can arrive in a single transaction. If the enote is spent it seems there is no call to query which transaction spent it.

The wallet does not store incoming transactions with at least 1 enote as as separate dedicated list, only in the form of transfer_details.m_tx where we can have duplicates. Once full transactions were part of this struct until it was recognized that this is not needed and only a waste of space; since then not the full transaction, but only its prefix is here.

The member variable to hold this list is m_transfers:

transfer_container m_transfers;

The implementation of get_transfers is therefore an absolutely trivial single line:

incoming_transfers = m_transfers;

Incoming confirmed payments

void get_payments(
  std::list<std::pair<crypto::hash,wallet2::payment_details>>& payments,
  uint64_t min_height,
  uint64_t max_height = (uint64_t)-1,
  const boost::optional<uint32_t>& subaddr_account = boost::none,
  const std::set<uint32_t>& subaddr_indices = {}) const;

struct payment_details
{
  crypto::hash m_tx_hash;
  uint64_t m_amount;
  amounts_container m_amounts;
  uint64_t m_fee;
  uint64_t m_block_height;
  uint64_t m_unlock_time;
  uint64_t m_timestamp;
  bool m_coinbase;
  cryptonote::subaddress_index m_subaddr_index;
}

typedef std::vector<uint64_t> amounts_container; 

This gives back all payments received with incoming confirmed transactions. A payment in the sense of this call is the sum of all enotes of such a transaction that went to a particular subaddress with incoming confirmed transaction tx_hash. The name of this method would be clearer if it contained the term incoming somewhere.

With a typical transaction it's pretty simple: It contains a single enote to one subaddress, and in this case that enote is already the payment, and in a certain sense also the transaction because the enote with the change does not interest at all. Transactions that contain enotes going to several of our subaddresses are somewhat special, and several enotes going to the same subaddress even more so, but of course possible because a transaction can contain any combination of up to 16 enotes.

It's interesting that only wallet2 has a dedicated method for merely getting these payments in particular, and also that its use of the term payment stands out:

In the CLI wallet you get them if you use the command show_transfers with type in included. RPC wallet COMMAND_RPC_GET_TRANSFERS gives them back if you set the in boolean in the request and works with a transfer_entry struct that covers all possible types of payments, not only incoming confirmed ones. wallet2 API includes them in a TransactionHistory object, marked as Direction_In, also working with a struct called TransactionInfo that covers all possible types of payments. The Monero C++ library has a get_transfers method that depending on the query can only give back incoming confirmed payments as a list of monero_transfer structs.

The struct member m_amounts detailing the amounts in the case of several enotes to the same subaddress is a relatively new addition.

The first element of the pair is the payment id. Not including this as a member of payment_details and not giving back the payments as a simple list looks strange but is probably a consequence of the way wallet2 stores these payments internally, to support lookup by payment id:

payment_container m_payments;

typedef serializable_unordered_multimap<crypto::hash, payment_details> payment_container;

Incoming confirmed payments for payment id

void get_payments(
  const crypto::hash& payment_id,
  std::list<wallet2::payment_details>& payments,
  uint64_t min_height = 0,
  const boost::optional<uint32_t>& subaddr_account = boost::none,
  const std::set<uint32_t>& subaddr_indices = {}) const;

This returns all incoming confirmed payments that have the given payment id. It's the basis for the CLI wallet command payments and RPC wallet COMMAND_RPC_GET_PAYMENTS. It's a "convenience method" with low importance as you could easily use the first get_payments method above and pick payments by id yourself.

Incoming unconfirmed payments

void get_unconfirmed_payments(
  std::list<std::pair<crypto::hash,wallet2::pool_payment_details>>& unconfirmed_payments,
  const boost::optional<uint32_t>& subaddr_account = boost::none,
  const std::set<uint32_t>& subaddr_indices = {}) const;

struct pool_payment_details
{
  payment_details m_pd;
  bool m_double_spend_seen;
}

struct payment_details
{
  crypto::hash m_tx_hash;
  uint64_t m_amount;
  amounts_container m_amounts;
  uint64_t m_fee;
  uint64_t m_block_height;
  uint64_t m_unlock_time;
  uint64_t m_timestamp;
  bool m_coinbase;
  cryptonote::subaddress_index m_subaddr_index;
}

typedef std::vector<uint64_t> amounts_container;

This returns all incoming unconfirmed payments. It introduces yet another struct pool_payment_details for just one member more than payment_details that the get_payments method for confirmed payments uses.

Again the other systems give back these payments as part of some unified system as pool - for the general approach check comments further up.

The container to store these:

serializable_unordered_multimap<crypto::hash, pool_payment_details> m_unconfirmed_payments;

Outgoing confirmed transactions

void get_payments_out(
  std::list<std::pair<crypto::hash,wallet2::confirmed_transfer_details>>& confirmed_payments,
  uint64_t min_height,
  uint64_t max_height = (uint64_t)-1,
  const boost::optional<uint32_t>& subaddr_account = boost::none,
  const std::set<uint32_t>& subaddr_indices = {}) const;

struct confirmed_transfer_details
{
  cryptonote::transaction_prefix m_tx;
  uint64_t m_amount_in;
  uint64_t m_amount_out;
  uint64_t m_change;
  uint64_t m_block_height;
  std::vector<cryptonote::tx_destination_entry> m_dests;
  crypto::hash m_payment_id;
  uint64_t m_timestamp;
  uint64_t m_unlock_time;
  uint32_t m_subaddr_account;   // subaddress account of your wallet to be used in this transfer
  std::set<uint32_t> m_subaddr_indices;  // set of address indices used as inputs in this transfer
  std::vector<std::pair<crypto::key_image, std::vector<uint64_t>>> m_rings; // relative
}

struct tx_destination_entry
{
  std::string original;
  uint64_t amount;                    //money
  account_public_address addr;        //destination address
  bool is_subaddress;
  bool is_integrated;
}

This returns all outgoing confirmed transactions. Thus a bit confusingly here payment is indeed a full transaction and not merely info about a collection of enotes like with the get_payments methods to get incoming confirmed payments. Maybe things would also be a bit clearer if confirmed_transfer_details contained "out" in its name somehow.

Again the other systems give back these transactions as part of some unified system under out - for the general approach check comments further up. The RPC interface has a transfer_entry.destinations list member to store the possibly multiple destinations of an outgoing transaction that only gets used for such. wallet2 API has TransactionInfo.transfers for this, also working with a single struct for all types, and the Monero C++ library has monero_outgoing_transfer.m_destinations working with inheritance, as monero_outgoing_transfer is an extension of monero_transfer.

The container to store these:

serializable_unordered_map<crypto::hash, confirmed_transfer_details> m_confirmed_txs;

It's really a bit painful to see how the method name uses the term payment, the struct name the term transfer and the variable name the term transaction.

Outgoing unconfirmed transactions

void get_unconfirmed_payments_out(
  std::list<std::pair<crypto::hash,wallet2::unconfirmed_transfer_details>>& unconfirmed_payments,
  const boost::optional<uint32_t>& subaddr_account = boost::none,
  const std::set<uint32_t>& subaddr_indices = {}) const;

struct unconfirmed_transfer_details
{
  cryptonote::transaction_prefix m_tx;
  uint64_t m_amount_in;
  uint64_t m_amount_out;
  uint64_t m_change;
  time_t m_sent_time;
  std::vector<cryptonote::tx_destination_entry> m_dests;
  crypto::hash m_payment_id;
  enum { pending, pending_not_in_pool, failed } m_state;
  uint64_t m_timestamp;
  uint32_t m_subaddr_account;   // subaddress account of your wallet to be used in this transfer
  std::set<uint32_t> m_subaddr_indices;  // set of address indices used as inputs in this transfer
  std::vector<std::pair<crypto::key_image, std::vector<uint64_t>>> m_rings; // relative
}

This returns all outgoing unconfirmed transactions. The few and small differences between the two structs confirmed_transfer_details and unconfirmed_transfer_details begs the question why have two different structs in the first place.

Again the other systems give back these transactions as part of some unified system as pending and failed - for the general approach check comments further up.

The container to store these:

serializable_unordered_map<crypto::hash, unconfirmed_transfer_details> m_unconfirmed_txs;
DangerousFreedom1984 commented 1 year ago

I started looking at this issue this week and using the method 'divide to conquer' I think we could first separate the incoming and outcoming txs into separate discussions.

I want to talk a bit about the outgoing txs.

I agree that in wallet3 we could unify the confirmed_transfer_details and unconfirmed_transfer_details but I am not sure if it would be necessary as I dont know if we reached consensus that all wallet3 transaction outputs HAVE to be enotes (Seraphis standard). In this case, a module for tracking outputs would only actually track enotes.

There are three ways to populate (in memory or in a wallet file) the 'transaction_history_out' (the analogous of the 'confirmed_transfer_details'): 1) Storing the info just after making a transaction. 2) Scanning the blockchain for owned enotes with exposed key_images 3) Importing the records from a wallet file.

It is not possible to recover the full information of the outgoing transactions (for example to whom you sent to) by only scanning the blockchain (in a scenario where you recover your keys in a new computer for example). But anyway, this component should contain the most updated and complete information about the outgoing txs.

I see it being organized in a struct similar to that:

struct transaction_out
{
    // General information about transaction
    rct::key txid;
    rct::xmr_amount fee;

    // Network data
    std::string status;  // Failed, Pending or Out(confirmed)
    uint64_t block_height;
    time_t sent_time;
    uint64_t timestamp;
    uint64_t unlock_time;

    // Enotes used as inputs
    std::vector<sp::SpContextualEnoteRecordV1> sp_enote_records;
    std::vector<sp::LegacyContextualEnoteRecordV1> legacy_enote_records;

    // Destination and ring members of the tx
    std::vector<std::pair<std::string, uint64_t>> dest_amount;               // destination and amount
    std::vector<std::pair<crypto::key_image, std::vector<uint64_t>>> rings;  // ring members
};

struct transaction_history_manager
{
    // a map may be better.
    // TODO: find fastest way to go from txid -> enotes
    std::vector<transaction_out> tx_history_manager;

    // Add methods to:
    // - get entries
    // - check duplicated entries
    // - update entries
    // - remove unnecessary entries
    // - optimzed queries, etc
};

To get an idea, I prototyped a function to add a tx to the tx manager and a function to show the content of it.

... (one day later)...

Now that I am thinking about it again, I am not really sure if we really need a component like that since in Seraphis the class SpEnoteStoreMockV1 (used to track the enotes) is capable of providing all the information contained in that struct. Maybe better things to think about are:

I will be looking again on the whole issue in the next days.

rbrunner7 commented 1 year ago

I can't comment about your design thoughts yet, but for now just want to throw a thought of mine into the ring that might get overlooked:

When designing the wallet components that hold info like enotes and transactions we should not merely care about the wallet itself, but also think about all the possible clients that want to get info from the wallet about what happened.

wallet2 demonstrates that there is info that the wallet itself won't ever need, or won't ever need again, to do all its work, especially regarding outgoing transactions, and thus can technically get away with not storing it. But if the info is not there, and a client has a good use for it, things can get very, very hard, which is unfortunate.

In my TechWallet I want people to show as clearly as possible what happens on the enote level. For that I need the info for a spent enote which transaction spent it. It seems that walle2 does not store that info, and I guess that even makes sense from a purely "inside" point of view: It doesn't need that info to function itself, and "normal" wallets don't want to query and display it either.

I had to resort to quite extreme measures to get the desired info, just because wallet2 doesn't do me the favor to hold and tell it: I now scrap HTML returned from a block explorer ...

rbrunner7 commented 1 year ago

One comment nevertheless, about this here in your mock definitions:

// Enotes used as inputs
std::vector<sp::SpContextualEnoteRecordV1> sp_enote_records;
std::vector<sp::LegacyContextualEnoteRecordV1> legacy_enote_records;

This is less than elegant right now already, and doesn't this look like it will develop into some big mess over time, quite in principle? What happens if Seraphis evolves and comes up with EnoteRecordV2, V3, V4, whatever?

Maybe for enotes the same question is appropriate like I ask it for transactions, proposed to discuss tomorrow: Maybe some "generalized", "over-arching" class able to hold any enote would be a very good idea?

DangerousFreedom1984 commented 1 year ago

This is less than elegant right now already, and doesn't this look like it will develop into some big mess over time, quite in principle? What happens if Seraphis evolves and comes up with EnoteRecordV2, V3, V4, whatever?

Agree. But before going into the details I will think more about the members and shape of this struct. The class SpEnoteStoreMockV1 contains really a lot of information and it could be almost itself the 'transaction_manager'.

UkoeHB commented 1 year ago

The enote store should not expand its capabilities beyond its current state. Its purpose is storing enotes loaded from balance-recovery processes. The internal complexity is already quite high to achieve that, adding more would overburden the class.

UkoeHB commented 1 year ago

This is less than elegant right now already, and doesn't this look like it will develop into some big mess over time, quite in principle? What happens if Seraphis evolves and comes up with EnoteRecordV2, V3, V4, whatever?

If necessary you can use a variant of records.

DangerousFreedom1984 commented 1 year ago

Agree with that too. So the trick will be to get the info we request in the most optimized way apparently.

UkoeHB commented 1 year ago

So the trick will be to get the info we request in the most optimized way apparently.

Yes, the strategy I decided on is to build secondary representations of the enote store contents using whatever caching strategy makes the most sense for each use-case. When an enote store is updated, it will emit a list of 'EnoteStoreEvents' that can be used to build the secondary representations.

If no special caching methods are needed, you can just query the enote store directly.

UkoeHB commented 1 year ago

struct transaction_out

struct TransactionRecordV1
{
    rct::key txid;
    rct::xmr_amount fee;

    SpSpentContextV1 spent_context;  //contains state of the record

    // key images of spent enotes for tracking purposes
    std::vector<crypto::key_image> legacy_spent_enotes;
    std::vector<crypto::key_image> sp_spent_enotes;

    // input rings record (why is this needed?)
    std::unordered_map<crypto::key_image, std::vector<std::uint64_t>> legacy_reference_sets;
    std::unordered_map<crypto::key_image, SpBinnedReferenceSetV1> sp_reference_sets;

    // sent funds
    std::vector<std::pair<JamtisDestinationV1, rct::xmr_amount>> outlays;
};
DangerousFreedom1984 commented 1 year ago

I dont think that the input rings record are needed as they can be easily retrieved if needed for something. So in your TransactionRecordV1 proposal you are not storing the enotes (which for me was the most important part). So from where do you expect to recover them? Your idea is to store the SpEnoteStoreMockV1 directly in the wallet files and use basically the TransactionRecordV1 as an accessory to show transaction info only (pretty much like the transfer_view)? If an info from an enote is needed then the transaction_manager (or the knowledge_proofs) would query the SpEnoteStoreMockV1 to get it. Is it how you see it? I think it makes sense that way.

UkoeHB commented 1 year ago

So in your TransactionRecordV1 proposal you are not storing the enotes (which for me was the most important part).

Enote records contain more information than the tx record needs, so it's better to just store an identifier like the key image that can be used to find the enote later.

Your idea is to store the SpEnoteStoreMockV1 directly in the wallet files and use basically the TransactionRecordV1 as an accessory to show transaction info only (pretty much like the transfer_view)?

My idea is to store a tools::readable<SpEnoteStoreV1> in the wallet files that can be used for read-only access to the store. The mutable handle will be owned by code that handles enote store updates.

DangerousFreedom1984 commented 1 year ago

Yeah, seems the best approach since the SpContextualEnoteRecordV1 has all the information of a tx and querying it directly makes more sense. So a transaction_history component would only actually query the enotes and store the information it needs in the wallet files, right? But what about the other way round ? If a wallet retrieves an updated transaction_history component then would it be able to update the enotes?

UkoeHB commented 1 year ago

If a wallet retrieves an updated transaction_history component then would it be able to update the enotes?

I don't follow. Transaction history should be an isolated component.

DangerousFreedom1984 commented 1 year ago

Ok. Let me try to better shape it and confirm that we are on the same page.

1) So the THC (Transaction history component) or THM (Transaction history manager) will be a separated wallet module that would only be updated by the EnoteStore or the wallet files, right? There should be a method to sync it with the EnoteStore (slow) and a method to read the last state (fast) which is getting the info from the wallet file. If so, there may be conflicts here so a second discussion would be how to address them.

2) I believe the following structures are quite optimized if we want to go from a specific txid to an enote. Let me know your thoughts.

struct TransactionOutRecordV1
{
    rct::key txid;
    rct::xmr_amount fee;

    // map<txid,key_image>
    // key images of spent enotes for tracking purposes
    std::vector<crypto::key_image> legacy_spent_enotes;
    std::vector<crypto::key_image> sp_spent_enotes;

    sp::SpEnoteSpentStatus spent_status;  // contains state of the record

    // sent funds
    std::vector<std::pair<JamtisDestinationV1, rct::xmr_amount>> outlays;
};

struct TransactionHistoryManager
{

    // use unordered_multimap for two reasons:
    // 1. quickly find TxRecords from txid
    // 2. one txid can contain multiple TransactionOutRecord
    std::unordered_multimap<rct::key, TransactionOutRecordV1> tx_history_manager2;

    // use sorted_map by blockheight to find last transactions or txs 
    // in a specific time range
    std::map<std::uint64_t, rct::key> tx_id;

    // Add methods to:
    // The idea is to fill the THM with the info from the EnoteStore
    // - build TransactionHistoryManager from SpEnoteStoreMockV1
    // - build THM from file (tentative)
    // - get vector<SpContextualEnoteRecord> of specific tx 
    // - get last N txs (for showing)
    // - get balance of spent tx
    // - get outlays of spent tx 
        // SpEnoteSpentContextV1 does not store this information. Ideally it should be there.
        // Retrieve info directly from THM?
        // This is useful for example for enote_ownership_proof_sender_plan

    // - handle submitted/unconfirmed/confirmed (use self-send for tracking?)
    // These functions depends on the daemon, some cross work is needed here.

    // - to be completed... 

    // make knowledge proofs (go from txid to EnoteRecords when needed)
    // these functions should be ready for wallet integration, which means they
    // could be called directly from the CLI commands

    // - address_ownership
    // - address_index
    // - enote_ownership
    // - enote_amount
    // - enote_key_image
    // - unspent_proof 
        // tentative -> think about optimized solution to scan blockchain and tell if enote appeared as a ring member
    // - tx_funded
    // - enote_sent
    // - reserve_proof

};

3) The only missing information are the outlays that are not stored in the EnoteContext. I suggest you to add it to SpEnoteSpentContextV1. This would also be useful to the address book module later. What do you think?

4) This component should do the same as these wallet2 functions at least:

    void get_transfers(wallet2::transfer_container& incoming_transfers) const;
    void get_payments(const crypto::hash& payment_id, std::list<wallet2::payment_details>& payments, uint64_t min_height = 0, const boost::optional<uint32_t>& subaddr_account = boost::none, const std::set<uint32_t>& subaddr_indices = {}) const;
    void get_payments(std::list<std::pair<crypto::hash,wallet2::payment_details>>& payments, uint64_t min_height, uint64_t max_height = (uint64_t)-1, const boost::optional<uint32_t>& subaddr_account = boost::none, const std::set<uint32_t>& subaddr_indices = {}) const;
    void get_payments_out(std::list<std::pair<crypto::hash,wallet2::confirmed_transfer_details>>& confirmed_payments,
      uint64_t min_height, uint64_t max_height = (uint64_t)-1, const boost::optional<uint32_t>& subaddr_account = boost::none, const std::set<uint32_t>& subaddr_indices = {}) const;
    void get_unconfirmed_payments_out(std::list<std::pair<crypto::hash,wallet2::unconfirmed_transfer_details>>& unconfirmed_payments, const boost::optional<uint32_t>& subaddr_account = boost::none, const std::set<uint32_t>& subaddr_indices = {}) const;
    void get_unconfirmed_payments(std::list<std::pair<crypto::hash,wallet2::pool_payment_details>>& unconfirmed_payments, const boost::optional<uint32_t>& subaddr_account = boost::none, const std::set<uint32_t>& subaddr_indices = {}) const;

    size_t get_num_transfer_details() const { return m_transfers.size(); }
    const transfer_details &get_transfer_details(size_t idx) const;

    std::string get_tx_proof(const crypto::hash &txid, const cryptonote::account_public_address &address, bool is_subaddress, const std::string &message);
    std::string get_tx_proof(const cryptonote::transaction &tx, const crypto::secret_key &tx_key, const std::vector<crypto::secret_key> &additional_tx_keys, const cryptonote::account_public_address &address, bool is_subaddress, const std::string &message) const;
    bool check_tx_proof(const crypto::hash &txid, const cryptonote::account_public_address &address, bool is_subaddress, const std::string &message, const std::string &sig_str, uint64_t &received, bool &in_pool, uint64_t &confirmations);
    bool check_tx_proof(const cryptonote::transaction &tx, const cryptonote::account_public_address &address, bool is_subaddress, const std::string &message, const std::string &sig_str, uint64_t &received) const;

    std::string get_spend_proof(const crypto::hash &txid, const std::string &message);
    bool check_spend_proof(const crypto::hash &txid, const std::string &message, const std::string &sig_str);
    std::string get_reserve_proof(const boost::optional<std::pair<uint32_t, uint64_t>> &account_minreserve, const std::string &message);
    bool check_reserve_proof(const cryptonote::account_public_address &address, const std::string &message, const std::string &sig_str, uint64_t &total, uint64_t &spent);

    std::pair<uint64_t, std::vector<tools::wallet2::exported_transfer_details>> export_outputs(bool all = false) const;
    std::string export_outputs_to_str(bool all = false) const;
    size_t import_outputs(const std::pair<uint64_t, std::vector<tools::wallet2::exported_transfer_details>> &outputs);
    size_t import_outputs(const std::pair<uint64_t, std::vector<tools::wallet2::transfer_details>> &outputs);
    size_t import_outputs_from_str(const std::string &outputs_st);
    payment_container export_payments() const;
    void import_payments(const payment_container &payments);
    void import_payments_out(const std::list<std::pair<crypto::hash,wallet2::confirmed_transfer_details>> &confirmed_payments);

There may be more or different ones but that's the direction that this component should go. What do you think?

UkoeHB commented 1 year ago

There should be a method to sync it with the EnoteStore (slow) and a method to read the last state (fast) which is getting the info from the wallet file. If so, there may be conflicts here so a second discussion would be how to address them.

These don't need to be methods on the component. I'd encourage implementing as lean a component as possible.

How to resolve conflicts is something you'll have to figure out as the engineer. There is potential for conflict between all of these: records loaded from file, information found in an enote store on startup, records added while the program is running, enote store updates while the program is running.

TransactionOutRecordV1

Just call this TransactionRecordV1. Including Out is just ambiguous.

sp::SpEnoteSpentStatus spent_status;

Would be much more useful to record the entire spent context. Plus you need the full context if you want to use e.g. try_update_enote_spent_context_v1().

// 2. one txid can contain multiple TransactionOutRecord

This should be impossible. If you encounter multiple conflicting tx records, you need to have a solution ready to merge them.

std::map<std::uint64_t, rct::key> tx_id;

Is this a map of timestamps? It's not documented.

// - get last N txs (for showing)

I think boost has a utility for storing a range of references. It may be worth looking into, so you don't need to make copies of all txs in a case like this.

// - get balance of spent tx // - get outlays of spent tx

These shouldn't need to be methods of the manager. The manager can expose 'lookup' functions for specific txs or ranges of txs, then you can use free functions to do needed operations on acquired txs (like getting full balance).

The reason to minimize the interface of the manager is to reduce direct accesses to the manager's member variables, since over-abundant member access is a source of technical debt/spaghetti. It also makes it relatively harder to reason about the methods involved since the internal state of an object is controlled by intricate and dispersed invariants whereas its interface's invariants are direct and specific.

// SpEnoteSpentContextV1 does not store this information. Ideally it should be there.

What information? Outlays? Outlays cannot be recovered from the blockchain.

// - handle submitted/unconfirmed/confirmed (use self-send for tracking?) // These functions depends on the daemon, some cross work is needed here.

Access to the daemon is not needed. Enote stores will emit an enote store event list when they are updated. Use the store events plus a const ref to the store to identify transaction records that need an updated spent context. Look up key images from the store events in the manager (e.g. ClearedLegacySpentContext, ClearedSpSpentContext, UpdatedLegacySpentContext, UpdatedSpSpentContext, etc.), then ask the enote store for the associated enote records, then import the updated spent contexts to the manager.

// make knowledge proofs (go from txid to EnoteRecords when needed)

All of these should work as free functions. The manager just needs to be a cache that focuses on managing its internal state and providing access to that state in a clean/efficient manner.

The only missing information are the outlays that are not stored in the EnoteContext. I suggest you to add it to SpEnoteSpentContextV1. This would also be useful to the address book module later. What do you think?

Like I said, outlays cannot be recovered from the blockchain so it's not appropriate to store them in spent contexts. Outlays can only be recorded when you are creating a transaction (only records of txs that get submitted should be recorded).

There may be more or different ones but that's the direction that this component should go. What do you think?

I think you're on the right track, as long as you don't over-burden the manager. One piece of advice: don't force yourself to implement all of those functions if they have complicated dependencies (e.g. needing to request information from the daemon). First implement the easy ones that can be completed with just the available tools, then we can review what's left over and see if there is a 'next task/component' that can support missing functionality.

DangerousFreedom1984 commented 1 year ago

These don't need to be methods on the component. I'd encourage implementing as lean a component as possible.

I agree. I will try to make the simplest function (show some records) as soon as we agree with the minimal structure.

How to resolve conflicts is something you'll have to figure out as the engineer. There is potential for conflict between all of these: records loaded from file, information found in an enote store on startup, records added while the program is running, enote store updates while the program is running.

Yeah I know, just making sure we understand that.

Just call this TransactionRecordV1. Including Out is just ambiguous.

What about the incoming transactions? Should this module keep track of the txs with unspent enotes? I didnt think much about it but seems to be a good idea. Anyway I will think about it later.

What information? Outlays? Outlays cannot be recovered from the blockchain. Like I said, outlays cannot be recovered from the blockchain so it's not appropriate to store them in spent contexts. Outlays can only be recorded when you are creating a transaction (only records of txs that get submitted should be recorded).

Okay, I was not sure about the correct approach here. You could indeed create a field in the EnoteContext to update the EnoteStore when a transaction is made but then I guess it would generate more conflicts if you would try to reload in different places... You are right here, it is better to be simple and coherent and let the EnoteStore recover only information available from the blockchain. The wallet should keep the records of the outlays.

This should be impossible. If you encounter multiple conflicting tx records, you need to have a solution ready to merge them. Is this a map of timestamps? It's not documented.

Oh I'm stupid sorry. I thought one thing and wrote another. So, the idea is to go as optimized as possible from: blockheight -> tx_ids -> TransactionRecord. One blockheight can contain multiple tx_ids but every tx_id refers to only one TransactionRecord of course.

I thought about storing a multimap<blockheight,tx_ids> and an unordered_map<tx_ids,TransactionRecord> so if I want to select a range (or the last txs) then I would first query the variable multimap<blockheight,tx_ids> txids and then from the txids I would get the TransactionRecords looking at the unordered_map<tx_ids,TransactionRecord> tx_records. You mean that using boost_range there is a faster way to do it? I will see how this can be better done.

Access to the daemon is not needed.

Cool. I will make use of the EnoteStore a lot and don't really care about its management for now.

I think you're on the right track, as long as you don't over-burden the manager. One piece of advice: don't force yourself to implement all of those functions if they have complicated dependencies (e.g. needing to request information from the daemon). First implement the easy ones that can be completed with just the available tools, then we can review what's left over and see if there is a 'next task/component' that can support missing functionality.

Ok, thanks for the answers. I will make the simplest implementations and then we can discuss more about it next week.

UkoeHB commented 1 year ago

What about the incoming transactions? Should this module keep track of the txs with unspent enotes? I didnt think much about it but seems to be a good idea. Anyway I will think about it later.

There is no such thing as an incoming transaction. I feel a little crazy reading this paragraph.

So, the idea is to go as optimized as possible from: blockheight -> tx_ids -> TransactionRecord. One blockheight can contain multiple tx_ids but every tx_id refers to only one TransactionRecord of course.

If you want to go block height -> tx id, just use a std::map<height, std::set<tx id>> or equivalent.

and then from the txids I would get the TransactionRecords looking at the unordered_map<tx_ids,TransactionRecord> tx_records. You mean that using boost_range there is a faster way to do it? I will see how this can be better done.

To do 'I would get the TransactionRecords' you would normally need to allocate a vector or list of elements to return all the data. I am saying you could instead just return a container of references to your elements, which may be more efficient.

DangerousFreedom1984 commented 1 year ago

There is no such thing as an incoming transaction. I feel a little crazy reading this paragraph.

Yeah, all the information you get from an incoming transaction can be found in the EnoteStore. So better call it incoming enotes instead of transactions. So yeah, nothing to do here.

To do 'I would get the TransactionRecords' you would normally need to allocate a vector or list of elements to return all the data. I am saying you could instead just return a container of references to your elements, which may be more efficient.

Okay, I will try to find the optimum way and then we can discuss it. Thanks.

DangerousFreedom1984 commented 1 year ago

After thinking a couple of days, I believe I'm close to the most optimized and minimalist structure.

Statement of problem:

Let me begin comparing wallet2 and seraphis_wallet with an example where we retrieve the outgoing unconfirmed_payment (outlays and all the info related). Also works for confirmed txs.

image

So, if we want to show the (unconfirmed) txs chronologically then wallet2 would fill and recover this information in the serializable_unordered_multimap<crypto::hash, pool_payment_details> m_unconfirmed_payments and then copy this information to a vector and finally sort it. Which should be at least (n log n) in time. The space complexity is not great either as it is copying a lot of information to a new vector. Which is not really optimized if we want to sort it by blockheight or time.

Solution for seraphis_wallet:

To achieve this solution I propose the following class/structs:

class SpTransactionStoreV1
{
    // Quickly find TxRecords from txid
    std::unordered_map<rct::key, TransactionRecordV1> tx_records;

    // Sort by blockheight to find last transactions or txs 
    // in a specific time range
    std::map<std::uint64_t, std::set<rct::key>> confirmed_txids;

    // Multimap seems to be cleaner but less efficient -> to confirm
    // (Though not so many txs in the same block from the same wallet is expected)
    // std::multimap<std::uint64_t,rct::key> confirmed_txids;

    // Use timestamp instead of blockheight
    std::map<std::uint64_t, std::set<rct::key>> unconfirmed_txids;
    std::map<std::uint64_t, std::set<rct::key>> offchain_txids;

    // When a transfer is done:
    // - Entry will be created at tx_records to store outlays/key_images for a certain txid
    // - Enote_store will be updated
    // - Enote_store will issue a notification returning key_images of updated enotes
    // - SpTransactionStore will update confirmed_txids(by blockheight)/unconfirmed_txids/offchain_txids

    // Update:
    // - This component could be launched in a separated thread whenever a notification
    // to update is popped. So the confirmed/unconfirmed/offchain txs will always be updated.

    // methods to update tx_statuses

    // Show_transfers
    // Exhibit txs chronologically
    // - Get last N confirmed txs (ordered by blockheight)
    // - Get last N unconfirmed txs (ordered by timestamp)
    // - Get last N offchain txs (ordered by timestamp)
};

struct TransactionRecordV1
{
    // Key images of spent enotes for tracking purposes
    std::set<crypto::key_image> legacy_spent_enotes;
    std::set<crypto::key_image> sp_spent_enotes;

    // Sent funds
    std::set<std::pair<JamtisDestinationV1, rct::xmr_amount>> outlays;

    // Spent status is not necessary as it is stored in the variables which their names represent.
};

So, I would start implementing a basic show_transfers (to make use of all the structures and features) and in this case, if the user enters a tx id then I would go look at tx_records -> try_get_sp_enote_record and the enotes would be found in 2 log n. If the user wants to see the transactions in a specific time (block) range then I would go from confirmed_txids which outputs the txid (log n by blockheight) -> tx_records (log n by txid) -> try_get_sp_enote_record which outputs the enotes in 3 log n in total.

So before starting, I want to make sure that we are on the same wavelength with the following questions: 1) There is a big difference on the power that wallet2 has compared to sp_wallet. While wallet2 access directly the daemon, sp_wallet has and "engine" or "implementation" to handle enotes and other things. Should we strive to a wallet with very few (zero?) dependecy on the daemon and let this engine takes care of it? (I'm pretty sure of that). 2) In order to my design to work, I'm considering that whenever an update on the enote_store occurs then I would be able to receive a notification and retrieve the key_images updated and update the confirmed/unconfirmed/offchain txs. In this way, I would have the txs sorted by blockheight or timestamp. Is it a reasonable premise? Otherwise I could try to mess with the "engine" layer or make queries to enote_store to compare its state. I dont think these are good solutions though.

Let me know your thoughts and if we agree that I can proceed using the proposed scheme. If so, I think it is pretty clear to me the next tasks. I might have written some imprecise information but hopefully the line of thinking is clear.

UkoeHB commented 1 year ago

In order to my design to work, I'm considering that whenever an update on the enote_store occurs then I would be able to receive a notification and retrieve the key_images updated and update the confirmed/unconfirmed/offchain txs. In this way, I would have the txs sorted by blockheight or timestamp.

Right now I am thinking to use polling on async queue/channel where enote store update reports will be inserted. You don't have to worry about how exactly that will work just yet, since the method to update a transaction store doesn't need to care where the inputs come from.

Your design looks like it's going in the right direction.