ukoehb / monero Goto Github PK

I think the majority of these are useful for wallet users.

If all implemented, we would be able to easily maintain compatibility between the Seraphis lib and wallet2 (can import a Seraphis lib-based wallet into a complete and consistent wallet2-based wallet).

1) RCT / non-RCT status of enote

• In progress: #42
• Required to be able to spend pre-RCT using wallet2

2) Tx fee

• Answers "how much did I spend in fees" without re-consulting the chain

3) key_offsets of txs which include a received enote in their ring

• Useful to track how often an enote is used in other rings.

3a) Legacy unscanned txs which include a received enote as a ring member

• Useful for background syncing without the spend key (no need to re-query the daemon for key images to identify 0-change spends)

4) Coinbase boolean

• Helpful for miners
• Technically can be determined for RCT enotes if 1 is implemented (legacy RCT enotes with transparent amounts), not for pre-RCT

5) unlock_time on spend origin context

• Useful when the user sends to another user with 0 change, and wants to know when the tx will unlock without re-consulting the chain (for historical record keeping purposes)

6) key_offsets for spent enotes

• Useful to display the ring in the wallet UI

7) a tx's index in a block

• Useful to have a clear-cut correct answer to is_older_than for pre-RCT enotes in the same block (#42 (comment))
• Useful to create a consistently ordered wallet2 m_transfers container from a Seraphis enote store

8) the legacy cryptonote::transcaction

• wallet2-based wallets use the tx in a few ways beyond scanning:
  • It's expected in the wallet2 callbacks on_money_received, on_money_spent, on_skip_transaction
  • wallet2 saves the transaction prefix for each scanned enote in the wallet cache (https://github.com/monero-project/monero/blob/c8214782fb2a769c57382a999eaf099691c836e7/src/wallet/wallet2.h#L323). It then relies on it in downstream functions.
  • Feather expects the transaction prefix to be on some of the records saved in the wallet2 containers (monero-project@5770265)
• I see 2 options here:
  1. refactor where the tx is used where necessary (includes requiring downstream consumers who currently lean on the wallet2 callbacks to refactor)
  2. the Seraphis lib scanner can pass the tx along per identified enote, which can also be used to knock out 1, 2, 4, 5, and 6 from above
  • I think refactoring where the tx is used is fine and more in line with the design goals of the Seraphis lib, but our lives would probably be easier if we passed the tx along per-identified enote.

In some places (for example here), code specific to the ed25519 elliptic curve leaks into the Seraphis logic.

It would make sense to abstract that away. Makes more sense in the context of the proposal to switch to a different curve, but also would make it easier to check that the code matches the Seraphis specification.

On the sixth and final test_aggregated_bulletproof_plus performance test instance here, an exception is thrown with the message: sv/gamma are too large. The assertion statement can be found here, but I don't know enough about bulletproofs to debug it.

Would be nice if the scanner picked up payment ID's for users who use and have used the feature in the past.

Looks like it could be well suited for LegacyBasicEnoteRecord, LegacyIntermediateEnoteRecord, and LegacyEnoteRecord

Here's the logic wallet2 uses to identify and decrypt payment ID's: https://github.com/monero-project/monero/blob/c8214782fb2a769c57382a999eaf099691c836e7/src/wallet/wallet2.cpp#L2699-L2761

1. When there is >1 tx pub key included in tx extra:

   • wallet2 generates derivations for every tx pub key and checks every derivation against every output in each tx.
     • Source:
       • https://github.com/monero-project/monero/blob/ac02af92867590ca80b2779a7bbeafa99ff94dcb/src/wallet/wallet2.cpp#L2219-L2222
       • https://github.com/monero-project/monero/blob/ac02af92867590ca80b2779a7bbeafa99ff94dcb/src/wallet/wallet2.cpp#L3155-L3156
       • https://github.com/monero-project/monero/blob/ac02af92867590ca80b2779a7bbeafa99ff94dcb/src/wallet/wallet2.cpp#L3174-L3180
       • monero-project@11ae187
         • Note: it looks like this commit to scan using every tx pub key was added because there was a bug in cold wallets that used the 2nd pub key.
   • The Seraphis lib only checks the first tx pub key included in tx extra.
     • Source:
       • Source 1
       • Source 2
   • The problem: if someone crafts a non-standard tx using the nth tx pub key (where n > 1), then the Seraphis wallet lib wouldn't identify any receives in the tx but wallet2 would.
   • The proposed fix: the Seraphis lib could scan using all tx pub keys in tx extra.

2. When the first additional pub keys vector in tx extra has unequal number of pub keys than there are enotes in the tx:

   • wallet2 generates derivations for each additional pub key, and will then ignore those additional pub keys beyond the number of enotes in the tx and scan the tx normally.
     • Source:
       • https://github.com/monero-project/monero/blob/ac02af92867590ca80b2779a7bbeafa99ff94dcb/src/wallet/wallet2.cpp#L2224-L2230
       • https://github.com/monero-project/monero/blob/ac02af92867590ca80b2779a7bbeafa99ff94dcb/src/wallet/wallet2.cpp#L3157-L3158
       • https://github.com/monero-project/monero/blob/ac02af92867590ca80b2779a7bbeafa99ff94dcb/src/wallet/wallet2.cpp#L3170-L3178
       • https://github.com/monero-project/monero/blob/ac02af92867590ca80b2779a7bbeafa99ff94dcb/src/cryptonote_basic/cryptonote_format_utils.cpp#L1059-L1063
   • The Seraphis lib doesn't scan the tx.
     • Source:
       • Source 1
       • Source 2
   • The problem: if someone crafts a non-standard tx that includes an unequal additional pub keys than enotes in a tx, then the Seraphis wallet lib wouldn't identify any receives in the tx but wallet2 would.
   • The proposed fix: if there are more additional pub keys than enotes, the Seraphis lib could ignore the additional pub keys beyond the number of enotes in a tx. If there are fewer additional pub keys than enotes the Seraphis lib could just scan using additionals corresponding to the lower indexed enotes.

It is unsafe to use raw threadpool references everywhere. Instead, use std::shared_ptr.

The actual threadpool constructor can be hidden and only usable by a wrapper class that stores a shared_ptr of the threadpool. Then overload operator*/operator-> to dereference to the inner threadpool.

Continuing from point 2 in this comment in the async scanner: #23 (comment)

Ideally wallets minimize round trips to the daemon when loading. This was the idea behind returning pool txs in the /getblocks.bin endpoint: monero-project#8076

The Seraphis lib scanner currently does 1) scan chain -> 2) scan pool -> 3) scan chain in a scan pass, which would require a minimum of 3 round trips to the daemon. @UkoeHB mentioned:

I intentionally added a follow-up pass because there is a race condition between chain scanning and pool scanning that can cause mined blocks to contain txs invisible to a wallet even after it claims to have scanned the pool.

Proposed solution to this problem: ensure that when the /getblocks.bin endpoint returns pool txs, those pool txs correspond to the state of the pool at the top_block_hash also returned by the endpoint (to be clear, this would be a change to the daemon RPC side). This way the scanner can be certain that once it has scanned up to chain tip, it knows the state of the pool at that time.

Some issues with the TXID calculation:

1. Doesn't explain why the input key images are hashes twice. IMO it's sufficient to have them in the prefix, which would save 1 hash.
2. The final hash hashes more than 128 bytes of data, which requires two calls to the Blake2b compression function. This has the same performance impact as one additional hash. Since 3 hashes take 96 bytes, the domain separation stuff could still fit into the remaining 32 bytes if it was less verbose.