Modifications to enable Offline Signing in monero-gui and possible... #9492

DiosDelRayo · 2024-09-27T07:45:11Z

… other projects via UR or other mediums of exchange.

This PR is part of the CCS Monero Signer Resurrection (milestone 4). The modifications are needed to get the blobs of the offline signing data exchange without writing and reading from disk. To merge UR additions to monero-gui, this needs to be merged first.

   Added the following signatures in the mention files:

   wallet/wallet2.h
   +    std::string export_key_images_string(bool all = false) const;
   +    uint64_t import_key_images_string(const std::string &data, uint64_t &spent, uint64_t &unspent);
   M    bool wallet2::export_key_images(const std::string &filename, bool all) const

   wallet/api/pending_transaction.h
   +    std::string commit_string() override;

   wallet/api/unsigned_transaction.h
   +    std::string signAsString() override;

   wallet/api/wallet.h:
   +    bool submitTransactionFromString(const std::string &fileName) override;
   +    virtual UnsignedTransaction * loadUnsignedTxFromString(const std::string &unsigned_filename) override;
   +    std::string exportKeyImagesAsString(bool all = false) override;
   +    bool importKeyImagesFromString(const std::string &data) override;
   +    std::string exportOutputsAsString(bool all = false) override;
   +    bool importOutputsFromString(const std::string &data) override;

   wallet/api/wallet2_api.h
   +    virtual std::string commit_string() = 0;
   +    virtual std::string signAsString() = 0;
   +    virtual UnsignedTransaction * loadUnsignedTxFromString(const std::string &unsigned_filename) = 0;
   +    virtual bool submitTransactionFromString(const std::string &fileName) = 0;
   +    virtual bool importKeyImagesFromString(const std::string &data) = 0;
   +    virtual std::string exportOutputsAsString(bool all = false) = 0;
   +    virtual bool importOutputsFromString(const std::string &data) = 0;
   +    uint64_t import_key_images_string(const std::string &data, uint64_t &spent, uint64_t &unspent);

   And the implementations in:

   wallet/wallet2.cpp
   wallet/api/pending_transaction.cpp
   wallet/api/unsigned_transaction.cpp
   wallet/api/wallet.cpp

   The method `bool wallet2::export_key_images(const std::string &filename, bool all) const` is modified to
   use `std::string export_key_images_string(bool all = false) const;` to get the string to write to the file.
   IMO that would be the perfect way to do it everywhere, but in the other methods it would require more modifications, so the other I duplicated and removed the part writing to the file and return instead a std::string, or
   use a std::string for the actual payload instead of a file path.

   One thing to mention is I remove in one or two log messages the filename, and the other is in `export_key_images` probably(almost sure) is now the performance messed up.

   This modifications was done to get all the necessary data for offline signing via UR or any other channel not
   using files as medium. IMO it had been better to not implement the filehandling direct in wallet2 or in the wallet api but rather in monero-wallet-cli and monero-gui itself, but it is like it is.

…mages

…nd export_encrypted_key_images

…ther projects via UR or other mediums of exchange. Added the following signatures in the mention files: wallet/wallet2.h + std::string export_key_images_string(bool all = false) const; + uint64_t import_key_images_string(const std::string &data, uint64_t &spent, uint64_t &unspent); M bool wallet2::export_key_images(const std::string &filename, bool all) const wallet/api/pending_transaction.h + std::string commit_string() override; wallet/api/unsigned_transaction.h + std::string signAsString() override; wallet/api/wallet.h: + bool submitTransactionFromString(const std::string &fileName) override; + virtual UnsignedTransaction * loadUnsignedTxFromString(const std::string &unsigned_filename) override; + std::string exportKeyImagesAsString(bool all = false) override; + bool importKeyImagesFromString(const std::string &data) override; + std::string exportOutputsAsString(bool all = false) override; + bool importOutputsFromString(const std::string &data) override; wallet/api/wallet2_api.h + virtual std::string commit_string() = 0; + virtual std::string signAsString() = 0; + virtual UnsignedTransaction * loadUnsignedTxFromString(const std::string &unsigned_filename) = 0; + virtual bool submitTransactionFromString(const std::string &fileName) = 0; + virtual bool importKeyImagesFromString(const std::string &data) = 0; + virtual std::string exportOutputsAsString(bool all = false) = 0; + virtual bool importOutputsFromString(const std::string &data) = 0; + uint64_t import_key_images_string(const std::string &data, uint64_t &spent, uint64_t &unspent); And the implementations in: wallet/wallet2.cpp wallet/api/pending_transaction.cpp wallet/api/unsigned_transaction.cpp wallet/api/wallet.cpp The method `bool wallet2::export_key_images(const std::string &filename, bool all) const` is modified to use `std::string export_key_images_string(bool all = false) const;` to get the string to write to the file. IMO that would be the perfect way to do it everywhere, but in the other methods it would require more modifications, so the other I duplicated and removed the part writing to the file and return instead a std::string, or use a std::string for the actual payload instead of a file path. One thing to mention is I remove in one or two log messages the filename, and the other is in `export_key_images` probably(almost sure) is now the performance messed up. This modifications was done to get all the necessary data for offline signing via UR or any other channel not using files as medium. IMO it had been better to not implement the filehandling direct in wallet2 or in the wallet api but rather in monero-wallet-cli and monero-gui itself, but it is like it is.

@tobtoht

and needs to get enabled with `WITH_OTS_UR` to compile with UR. This update depends on [monero PR](monero-project/monero#9492). It cannot work without modifications on monero core! @tobtoht also mentions that build with static Qt5 will not work. I compiled and tested exclusively with ubuntu 24.04 in Qt Creator with Qt 5.15.2

sneurlax

Concept ACK and the code changes LGTM for the most part. Functions are separated, errors are handled well, and documentation via inline comments is in-line with the status quo. You could add more comments in areas, but the context around your changes also lacks comments, so they're in the established style in those cases--but could nevertheless be commented and I think in that case breaking convention/style would be justified. Unit tests could also be added, although this could be a good first issue for a subsequent contributor after this is merged.

SNeedlewoods

Ran out of time for a full review today, but for the first pass it's looking mostly good, left a few comments in case you want to look into things already before I finish the review.

Thanks for your contribution.

src/wallet/api/pending_transaction.cpp

SNeedlewoods · 2024-10-09T20:39:24Z

src/wallet/api/pending_transaction.cpp

+        LOG_ERROR(m_errorString);
+        m_status = Status_Error;
+    }
+    m_wallet.startRefresh();


Don't think this is needed.

SNeedlewoods · 2024-10-09T20:40:21Z

src/wallet/api/wallet.cpp

@@ -1134,6 +1134,27 @@ UnsignedTransaction *WalletImpl::loadUnsignedTx(const std::string &unsigned_file
  return transaction;
 }

+UnsignedTransaction *WalletImpl::loadUnsignedTxFromString(const std::string &data) {
+  clearStatus();
+  UnsignedTransactionImpl * transaction = new UnsignedTransactionImpl(*this);


Reminds me of this issue: #9461

src/wallet/wallet2.cpp

SNeedlewoods

Round 2, review complete.
From this round the only relevant change for code flow is setStatus() to setStatusError().
Other things may be more subjective and/or only style related.

src/wallet/api/wallet.cpp

src/wallet/api/wallet.h

SNeedlewoods · 2024-10-14T13:18:49Z

src/wallet/api/wallet.h

    virtual UnsignedTransaction * loadUnsignedTx(const std::string &unsigned_filename) override;
+    virtual UnsignedTransaction * loadUnsignedTxFromString(const std::string &unsigned_filename) override;


Keyword virtual should be used in the interface file wallet2_api.h only. (I know this is violated already in several places in the current API.)

And parameter name unsigned_filename should be data.

src/wallet/api/wallet2_api.h

(Method name: reason) signTxToStr: gets added in monero-project#9492 as `signAsString()` loadTx: not needed, all it does is `loadFromFile()` and `parseTxFromStr()` estimateBacklog(min_tx_weight, max_tx_weight, fees): not needed, calls `estimateBacklog(fee_levels)`

…2358265221

…monerogui-otsur

SNeedlewoods

Apart from what I wrote you on Matrix, here are two more minor things I found on this round of the review.
Will take another look later, but so far this is looking good, thanks for your work.

SNeedlewoods · 2024-12-02T19:50:56Z

src/wallet/api/wallet.cpp

+
+  bool r = m_wallet->parse_tx_from_str(data, transaction->m_pending_tx, NULL);
+  if (!r) {
+    setStatusError(Status_Ok, tr("Failed to load transaction from string"));


Suggested change

setStatusError(Status_Ok, tr("Failed to load transaction from string"));

setStatusError(tr("Failed to load transaction from string"));

SNeedlewoods · 2024-12-02T19:51:00Z

src/wallet/api/wallet.h

    virtual UnsignedTransaction * loadUnsignedTx(const std::string &unsigned_filename) override;
+    virtual UnsignedTransaction * loadUnsignedTxFromString(const std::string &data) override;


Suggested change

virtual UnsignedTransaction * loadUnsignedTxFromString(const std::string &data) override;

UnsignedTransaction * loadUnsignedTxFromString(const std::string &data) override;

MrCyjaneK · 2024-12-02T20:37:41Z

https://github.com/MrCyjaneK/monero_c/blob/master/patches/monero/0012-WIP-UR-functions.patch <- with functions that export UR strings redy to be displayed

who asks? and others added 15 commits July 17, 2024 20:09

add endpoints: export_encrypted_key_images and import_encrypted_key_i…

418adb7

…mages

add magic

54dab2e

fix /

86ba8e2

fix /

9b6499c

change to hex_to_pod

d2e3cff

change back and add debug code

5722f42

more debug code

67e634e

more debug code

2691395

more debug code

c5822d7

final modifications to adding endpoints import_encrypted_key_images a…

75c14c8

…nd export_encrypted_key_images

.gitignore

5e7c234

manual merge

e2fe45d

remove whitespace

25b0950

DiosDelRayo mentioned this pull request Sep 27, 2024

CCS Monero Signer Resurrection (milestone 4). UR offline signing data exchange. To merge, monero #9492 needs to be merged first. monero-project/monero-gui#4358

Open

selsta added pending review wallet labels Sep 28, 2024

sneurlax approved these changes Sep 30, 2024

View reviewed changes

SNeedlewoods reviewed Oct 9, 2024

View reviewed changes

SNeedlewoods reviewed Oct 14, 2024

View reviewed changes

DiosDelRayo mentioned this pull request Oct 23, 2024

Update PR: monero DiosDelRayo/DiosDelRayo.github.io#13

Closed

DiosDelRayo and others added 5 commits November 20, 2024 08:47

https://github.com/monero-project/monero/pull/9492#pullrequestreview-…

23d8934

…2358265221

https://github.com/monero-project/monero/pull/9492#pullrequestreview-…

744bf8d

…2358265221

https://github.com/monero-project/monero/pull/9492#pullrequestreview-…

bc76da9

…2358265221

https://github.com/monero-project/monero/pull/9492#pullrequestreview-…

c399de2

…2358265221

Merge branch 'monero-project:master' into monerogui-otsur

b8ad399

DiosDelRayo added 2 commits November 21, 2024 13:36

Merge remote-tracking branch 'origin' into monerogui-otsur

9e42096

Merge branch 'monerogui-otsur' of github.com:DiosDelRayo/monero into …

fa52496

…monerogui-otsur

SNeedlewoods reviewed Dec 2, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modifications to enable Offline Signing in monero-gui and possible... #9492

Modifications to enable Offline Signing in monero-gui and possible... #9492

DiosDelRayo commented Sep 27, 2024

sneurlax left a comment

SNeedlewoods left a comment

SNeedlewoods Oct 9, 2024

SNeedlewoods Oct 9, 2024

SNeedlewoods left a comment

SNeedlewoods Oct 14, 2024

SNeedlewoods left a comment

SNeedlewoods Dec 2, 2024

SNeedlewoods Dec 2, 2024

MrCyjaneK commented Dec 2, 2024

		virtual UnsignedTransaction * loadUnsignedTx(const std::string &unsigned_filename) override;
		virtual UnsignedTransaction * loadUnsignedTxFromString(const std::string &unsigned_filename) override;

	setStatusError(Status_Ok, tr("Failed to load transaction from string"));
	setStatusError(tr("Failed to load transaction from string"));

	virtual UnsignedTransaction * loadUnsignedTxFromString(const std::string &data) override;
	UnsignedTransaction * loadUnsignedTxFromString(const std::string &data) override;

Modifications to enable Offline Signing in monero-gui and possible... #9492

Are you sure you want to change the base?

Modifications to enable Offline Signing in monero-gui and possible... #9492

Conversation

DiosDelRayo commented Sep 27, 2024

sneurlax left a comment

Choose a reason for hiding this comment

SNeedlewoods left a comment

Choose a reason for hiding this comment

SNeedlewoods Oct 9, 2024

Choose a reason for hiding this comment

SNeedlewoods Oct 9, 2024

Choose a reason for hiding this comment

SNeedlewoods left a comment

Choose a reason for hiding this comment

SNeedlewoods Oct 14, 2024

Choose a reason for hiding this comment

SNeedlewoods left a comment

Choose a reason for hiding this comment

SNeedlewoods Dec 2, 2024

Choose a reason for hiding this comment

SNeedlewoods Dec 2, 2024

Choose a reason for hiding this comment

MrCyjaneK commented Dec 2, 2024