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

[problem] edge case when creating transactions to self #90

Open
spirobel opened this issue May 19, 2022 · 15 comments
Open

[problem] edge case when creating transactions to self #90

spirobel opened this issue May 19, 2022 · 15 comments

Comments

@spirobel
Copy link

spirobel commented May 19, 2022

When I parse a payment_uri with an integrated address like:
"monero:5F2AGUyXjR3bvX38GaAsARaQ1exeBCVkUbPTfGssBfXQ5VPor4eA144YpUE7FNg5bLcpEuehNne23MkG8ZMCkPEegecygCNXsmd1CnEenm?tx_amount=0.250000000000"
and pass the MoneroTxConfig to createTx I get this error:
Uncaught (in promise) RuntimeError: Aborted(Assertion failed: R == dbg_R, at: /Users/woodser/git/monero-javascript/external/monero-cpp/external/monero-project/src/crypto/crypto.cpp,450,generate_tx_proof). Build with -sASSERTIONS for more info.
I get the same error when I manually create a MoneroTxConfig with the integrated address.
So I assumed I ought to use decodeIntegratedAddress(integratedAddress) and pass the Standardaddress and PaymentId manually into the MoneroTxConfig. But when I do this, I get this error:

Error: Standalone payment IDs are obsolete. Use subaddresses or integrated addresses instead
    at LibraryUtils.WORKER_OBJECTS.<computed>.callbacks.<computed> (LibraryUtils.js:212:1)
    at Worker.LibraryUtils.WORKER.onmessage (LibraryUtils.js:188:1)

So it seems like the paymentId field for the MoneroTxConfig is the old style paymentId.
Creating transactions to standardaddresses works.

This was the code that I used on the server side to create the payment uri from this standard address:
75jZRMyxRMjbvX38GaAsARaQ1exeBCVkUbPTfGssBfXQ5VPor4eA144YpUE7FNg5bLcpEuehNne23MkG8ZMCkPEeTqtwPsG

async function(wallet, standardAddress, amount){
    wallet = await wallet
    let networkType = await wallet.getNetworkType();
    // get integrated address with randomly generated payment id
    await LibraryUtils.loadKeysModule();
    let integratedAddress = await MoneroUtils.getIntegratedAddress(networkType, standardAddress);
    //let paymentId = integratedAddress.getPaymentId(); // 16 characters write this to req.app.locals.payment or write it to the session
    let paymentUri = await wallet.createPaymentUri(new MoneroTxConfig().setAddress(integratedAddress.getIntegratedAddress()).setAmount(new BigInteger(amount)))
    return paymentUri;
}

maybe I messed up there somehow. It seems like I dont get out the same standard address that I put in. I will make a paymenturi with just a standard address and see if that works. EDIT: It works. So this issue most likely arises because I make faulty integrated addresses. I will verify again with a know good integrated address.
A known good integrated address produced a different error message: Invalid destination address at LibraryUtils.WORKER_OBJECTS.<computed>.callbacks.<computed> (LibraryUtils.js:212:1) at Worker.LibraryUtils.WORKER.onmessage (LibraryUtils.js:188:1)
so it seems like it still needs to be processed somehow before it can be fed into createTx.
EDIT again: this might be because the known good address is a mainnet address and I a m on stagenet. So maybe integrated addresses work fine, I just picked the wrong net. In either case the error message is less spooky
EDIT again AGAIN: I tried again with a stagenet integrated address. This time it worked without issues. So the issue is narrowed down to the integrated address creation process. I think it might lead to trouble because I use Moneroutils to generate an integrated address for a wallet that is different from the wallet that is currently opened with monerowalletfull. I want to do this because the backend should be able to create new integrated for addresses of users that we dont have the viewkeys for.
EDIT. the integrated address from above works when creating a transaction with the gui wallet, but it still produces the same error when creating a transaction with this library. I setloglevel to 10 but it didnt gain any new insights. I will try further and figure out where the issue is :-)

EDIT: I think I can reproduce it now: 1.create subaddress. 2. make integrated address with subaddress 3.create transaction to this integrated address. => leads to the crash with "Assertion failed: R == dbg_R," Conditions: it needs to be a subaddress of the same wallet. ( so essentially sending to ones own wallet subaddress with an embedded payment id leads to this it. side effect: decodeIntegrated address also leads to unexpected output with this address) Monero GUI also shows an error message for this. the question is how to best deal with it. getting users to create these tx leads to crashes. I will write a test case for this when I have more time.

@woodser
Copy link
Owner

woodser commented Jun 6, 2022

I was able to reproduce the issue. Thanks for reporting.

The problem is integrated addresses cannot be created from subaddresses.

monero-javascript-v0.7.1 is released which throws an error if a subaddress if provided. Thanks!

@woodser woodser closed this as completed Jun 6, 2022
@spirobel
Copy link
Author

spirobel commented Jun 6, 2022

related commit c49b791
I am sorry but I think this fixes the wrong thing. Creating integrated addresses from subaddresses is supported. It is just that the wallet-rpc for example does not implement this feature.
The issue is that somebody could still create these addresses with the old version or a different tool.
The real issue is that the createTx produces a crash in the edge case that we send a transaction to "ourselves".
If we createTx to a different wallet that used a subaddress to create an integrated address it works just fine.

Steps to reproduce:
1.create two wallets. 2.create an integrated address from a subaddress with wallet A. 3.create a transaction in wallet B to this address.
You will see it works if you use wallet B but it will produce the crash if you use wallet A ( the one that owns the subaddress)

the limitation to not allow integrated addresses from subaddresses does not exist in practice (as we can see everything works fine as long as it is a different wallet we send the money to). This limitation also shouldn't exist because there are valid usecases for this.

I have one of these usecases:
https://www.youtube.com/watch?v=4DLcsQ45zoE
I implemented this clubcards feature in my browser wallet ( will set the repo to public soon under MIT so everyone can see in detail how it works)
I want to give the users the ability to login to websites with a subaddress. The website will have monero-javascript running and will create integrated addresses for these users. (which will then be the payment addresses of these clubcards that other users of the website can buy) The website obviously shouldn't have the viewkeys of the users and they should also only have this one subaddress per user.

A better fix would be to detect if the integrated address we input to createTx is made from a subaddress that belongs to us and in this case make sure the crash does not happen by refusing to create the transaction.
An even better fix would be to get to the bottom of why this error happens in the monero codebase.
It is unclear why it is possible to send to these kind of addresses if it is a different wallet, but if it is the currently opened wallet it will lead to a crash.

Thank you very much and sorry for the inconvenience!

@woodser
Copy link
Owner

woodser commented Jun 7, 2022

I'm happy to support whatever's possible. I asked about integrated subaddresses on #monero-dev. Unfortunately it sounds like integrated subaddresses are not to be used:

woodser
are integrated subaddresses permissible and supportable? monero-wallet-rpc does not support it. one can manually call the same utilities to create integrated subaddresses, but there seems to be a problem when sending funds to self with one of these

moneromooo
No.
Subaddresses were made to avoid having the two part addresses, which confused users as they had two addresses that looked different but sent to the same address.
Well, that's one reason at least.

@spirobel
Copy link
Author

spirobel commented Jun 7, 2022

https://monerotech.info/Home/AddressInfo?address=5F2AGUyXjR3bvX38GaAsARaQ1exeBCVkUbPTfGssBfXQ5VPor4eA144YpUE7FNg5bLcpEuehNne23MkG8ZMCkPEegecygCNXsmd1CnEenm <- integrated address https://monerotech.info/Home/AddressInfo?address=55KVFgA389XbvX38GaAsARaQ1exeBCVkUbPTfGssBfXQ5VPor4eA144YpUE7FNg5bLcpEuehNne23MkG8ZMCkPEeTktAc8N <- "primary address" that was supposedly in there 😁 and the subaddress that was used to create the integrated address: https://monerotech.info/Home/AddressInfo?address=75jZRMyxRMjbvX38GaAsARaQ1exeBCVkUbPTfGssBfXQ5VPor4eA144YpUE7FNg5bLcpEuehNne23MkG8ZMCkPEeTqtwPsG as we can see the keys stay the same. The only difference is the prefix.
they also work in transactions ( except for this one special edge case when we send money to "ourselves" -> wallet sends money to its own subaddress -> this leads to a crash)
So the restriction to not allow subaddresses as a basis for integrated addresses is not there in practice.

there are also other deviations from the monero-wallet-rpc, so maybe this a similar opportunity to provide more options to developers. Have you taken a look at the video explantion of my use case? I think this will be a great feature for monero users 😅 This feature also does not break monero or interferes with others.

@woodser
Copy link
Owner

woodser commented Jun 7, 2022

Do we have any guarantees it works for all integrated subaddresses except when sending to self? This feature would diverge from official support which is not comfortable. We can't do anything that could put user funds at risk by sending to a malformed address. I don't know, so would need to defer to those more knowledgeable. Currently we have a core dev stating that integrated subaddresses are not to be used.

@spirobel
Copy link
Author

spirobel commented Jun 7, 2022

This feature would diverge from official support which is not comfortable.

what does official support mean? Is it documented somewhere?

Do we have any guarantees it works for all integrated subaddresses except when sending to self?

yes. I tried this both with monero-javascript and the monero-gui and it both worked. I can also try with wallet-rpc if you want, it will most likely work too.
You can see in the Address analyzer by brunner that I linked that it is all just public view and spend keys after all. The wallet cant tell the difference.

@woodser
Copy link
Owner

woodser commented Jun 7, 2022

By official support, I mean supported by the software in monero-project.

Monero GUI allows generating integrated subaddresses, or only sending to integrated subaddresses (which could be an oversight)? What happens if sending to self using an integrated subaddress in Monero GUI?

@spirobel
Copy link
Author

spirobel commented Jun 7, 2022

Monero GUI allows generating integrated subaddresses, or only sending to integrated subaddresses

Monero GUI does not support generating integrated addresses in any case because it is not geared towards merchants.

(which could be an oversight)

there is no way to stop it because the public view and spend keys inside the address are the same. You can compare the monerotech.info links above. ( it is the address that I mentioned in the issue in the beginning. It crashed when I sent from the same wallet, but sending from a different wallet worked.)

By official support, I mean supported by the software in monero-project.

I can double check if you want but most likely monero-wallet-rpc will create transactions to this kind of address too. Monero gui also builds from the core wallet api and your library imports from there too.

@woodser
Copy link
Owner

woodser commented Jun 7, 2022

If the Monero GUI crashes sending to self with an integrated subaddress, mind opening an issue on monero-project? In that case, monero-wallet-rpc will probably crash too. Even if integrated subaddresses aren't officially supported there, they shouldn't crash the wallet.

It would be helpful to include a link to a tool to generate the integrated subaddress or include a sample mnemonic and corresponding integrated subaddress.

@woodser woodser reopened this Jun 7, 2022
@spirobel
Copy link
Author

spirobel commented Jun 8, 2022

Even if integrated subaddresses aren't officially supported there, they shouldn't crash the wallet.

they dont crash the wallet. Neither monero-javascript nor the monero gui crashes when the "integrated subaddresses" belong to a different wallet. Everything works fine. The transaction is created and sent. Because the wallet cant tell the difference between the public keys of a primary address or a subaddress.

The only case where the error happens is when the "integrated subaddress" belongs to the same wallet the transaction is created on. That is a weird minor edge case and it seems like I am the first one that came across this. So I dont think it is necessary to introduce this artificial restriction that serves no purpose and means I have to maintain my own fork where these restrictions dont apply.

This is the error message that the GUI produces. It does not crash the whole program like with monero-javascript, but it certainly looks weird:
Screenshot from 2022-05-27 03-45-53

the proper fix for this is to 1.decode the integrated address when creating a transaction 2. check if the address belongs to the currently open wallet. 3. if that is the case show an error message and dont createTx.
It does not really address the root cause of the issue ( this is certainly a weird thing that shouldn't happen.), but that's the responsibility of core.

@woodser
Copy link
Owner

woodser commented Jun 8, 2022

I got more feedback for integrated subaddresses:

moneromooo
It would just fuck up the efforts people made to improve things, but it'd work.
If one wanted to avoid being an jerk about it, one might make their own custom address format that's obviously not monero, ie "custommagicstringHEREGOESPUBKEYS". That would go a long way towards making people realize those aren't monero addresses, just identifiers for whatever code uses them.
Or even something like moneroaddress+64bitnumber
Then it'd be obvious the addresses are the same when the extra payment id is removed.
In fact, monero:ADDRESS&payment_id=x
I knoew it'd come in handy at some point :D
Thought ought to be given to how to make it not dumb on chain. I'm not sure a dummy payment is added when all outs are subaddresses atm. If this is not the case, those txes would stick out.
But then I suppose the goal isn't to blend in if trying to ram a feature through on the side :/
There are two things really (that I remmeber off the top of my head). The user confusion and the on-chain distinguishibility.

Let's open an issue for the error when sending to self with an integrated address in Monero GUI / monero-wallet-rpc. The core wallet should be able to handle that edge case since the underlying keys are the same.

Re: "monero:ADDRESS&payment_id=x", do you know if sending to a subaddress with a separate payment ID works? IIRC the wallet prevents it.

In the meantime, we can re-enable generating them in the library and gracefully error when sending funds to self.

@spirobel
Copy link
Author

spirobel commented Jun 8, 2022

Re: "monero:ADDRESS&payment_id=x", do you know if sending to a subaddress with a separate payment ID works? IIRC the wallet prevents it.

this is a bad idea and should not be used because it is the "old style of using payment ids". Integrated addresses were introduced to solve exactly this issue, among others. (that is when the new 8 bit "compact" payment ids were introduced., they will be hidden in the transaction so external observers cant link it. Source: https://monerodocs.org/public-address/integrated-address/#integrated-addresses-vs-subaddresses

There is nothing new that needs to be implemented, as we can already send and create these kind of "integrated subaddresses" as you called it. There was just this little edge case bug that I reported.
I think maybe the title of this issue is a bit misleading because I didn't know what I was up against when I first started out. (it turned out to be a much smaller problem than expected)

@spirobel spirobel changed the title [problem] having issues creating transactions to integrated addresses. [problem] edge case when creating transactions to self Jun 8, 2022
@woodser
Copy link
Owner

woodser commented Jun 8, 2022

Opened an issue in monero-project: monero-project/monero#8380

@selsta
Copy link

selsta commented Jun 8, 2022

c49b791 seems like a correct fix. Integrated subaddresses are not supported by the core implementation for a variety of reasons, as explained by mooo's IRC comments.

Just because something is technically possible doesn't mean it's a good idea.

@spirobel
Copy link
Author

spirobel commented Jun 8, 2022

Just because something is technically possible doesn't mean it's a good idea.

maybe it would be better to block sending to primary addresses when they are part of integrated addresses. They are the thing that is legacy after all and not subaddresses.
This way we could also solve any distinguishability issues that might arise.

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

No branches or pull requests

3 participants