-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
[on hold] Disposable Address using Payment ID #1345
Conversation
@kenshi84, The issue I see is that these paymentID's aren't deterministic, so you need to backup them in case of a wallet crash. Rescanning is difficult etc. What about making the k's determistic? k0 = hash(a,00000000) When you need to refresh your wallet from block 0, your wallet is on the lookout for transactions withpaymentID k0. When that one is encountered, the wallet checks for k1 in the following blocks until k1 is encountered, etc This would mean that you still only need the viewkey to be able to see all incoming transactions. edit: this would solve the computational cost for checking transactions as well. |
@dnaleor There's no need to worry about backing up used payment IDs, because they are stored in the blockchain. |
@kenshi84 I know they are stored in the blockchain. I was talking about the wallet that needs to backup the paymentID's. If you don't do that, how is your wallet supposed to know that a certain transaction is sent to a one time address belonging to the your wallet when you need to refresh is from scratch? I have no idea if the computation you described (hash(a,k)-k) mod 256 == 0) is very fast while scanning? But I guess that using a deterministic paymentID makes scanning a lot faster. I could be wrong though. |
@dnaleor Restoring your wallet from scratch is really easy, believe me:) Basically, every time you see a new tx with 256bit pID |
The only thing that worries me is possibility of pID being omitted by accident. How do you recover the funds then? Rec: "Here's my address and pID, I will deliver the goods as soon as it's cleared" If Rec has his record of which pID he gave to Sen, he can check and recover the funds. However, if he lost his records, then what? Using @dnaleor 's deterministic scheme, he could try out first 1000 k's or so against the TX key. Once recovered, he should immediately send it to himself with the correct pID specified, to avoid having to go through the same process if he's later recovering from seed and make the next scan easier. But, if it's not deterministic, it's impossible to guess the |
If you use @dnaleor's idea of Another direction for improvement would be to integrate |
@kenshi84 & @JollyMort , what about making deterministic paymentID's [k[i]=hash(a,i)] that fit the original condition created by kenshi [(hash(a,k)-k) mod 256 == 0]? This solves the issue of an omitted paymentID by using a deterministisc paymentID, and keeps the option for just scanning the blockchain based on the mod256 condition (if that is in fact faster). Anyway, I really like your work, kenshi. This is basically a "meta stealth address scheme" you came up with ;) |
src/wallet/wallet2.h
Outdated
@@ -209,6 +209,8 @@ namespace tools | |||
|
|||
tx_construction_data construction_data; | |||
|
|||
bool is_onetime; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm worried about potential inconsistencies in the code outside simplewallet that uses wallet2::commit_tx(ptx)
, specifically:
src/wallet/api/pending_transaction.cpp
src/wallet/wallet_rpc_server.cpp
I've never touched the RPC server etc., so I have no idea what side effects I might have caused.
@dnaleor No, whether it satisfies the equation or not is irrelevant here. I guess with the new version where the payment ID is integrated, we don't need to worry about forgetting to put the 256bit payment ID. |
Well, in theory yes. But I can imagine someone doing a bad wallet implementation and sending the XMR to the address without the attached paymentID... Maybe an edge case and worth the risk, but still a trade off. |
@dnaleor At least I'm personally happy with this implementation; if someone thinks a better solution is needed, he's gonna just implement it himself :) |
@kenshi84 any chance you could do a formal write-up and submit it to the Monero Research Lab repo for us to publish? It would go as MRL-0006, and the current WIP that is MRL-0006 would be moved out to 7 or something. |
@fluffypony Sure, I'd be honored to be able to publish at MRL:) |
@@ -504,7 +504,7 @@ namespace cryptonote | |||
if (parse_tx_extra(tx.extra, tx_extra_fields)) | |||
{ | |||
tx_extra_nonce extra_nonce; | |||
if (find_tx_extra_field_by_type(tx_extra_fields, extra_nonce)) | |||
if (!is_onetime && find_tx_extra_field_by_type(tx_extra_fields, extra_nonce)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: this prevents the encryption for the one-time mode.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks mostly nice. See comments inside. Please rebase/squash commits. Misc things are missing (RPC, mostly), so I'll probably have to add that before this gets merged. The way key images are selected needs further in depth review to ensure we can't double spend. Thanks for the patch!
@@ -1040,8 +1040,6 @@ bool simple_wallet::init(const boost::program_options::variables_map& vm) | |||
} | |||
crypto::secret_key viewkey = *reinterpret_cast<const crypto::secret_key*>(viewkey_data.data()); | |||
|
|||
m_wallet_file=m_generate_from_view_key; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why remove this ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because the same thing appears at #L1011. Isn't this line redundant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like it is.
src/simplewallet/simplewallet.cpp
Outdated
@@ -2227,6 +2226,7 @@ bool simple_wallet::transfer_main(int transfer_type, const std::vector<std::stri | |||
else while (!ptx_vector.empty()) | |||
{ | |||
auto & ptx = ptx_vector.back(); | |||
ptx.is_onetime = is_onetime; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That seems wrong. The wallet tx creation should set this if it creates such a tx. The flag is passed to it already.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be. I did this in order to avoid decrypting payment ID in wallet2::get_payment_id()
when in the one-time mode, but actually I don't really know how wallet2::commit_tx()
works. In that function, payment_id
seems to be used only in wallet2::add_unconfirmed_tx()
, but what's the purpose of this function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be irrelevant to get_payment_id. Functions like create_transactions_2 fill up the ptx set, and should set the is_onetime from the passed parameter. This is done just before in the timeline, so get_payment_id does not need to change.
add_unconfirmed_tx saves data about an outgoing tx that cannot be recovered from the blockchain (for examaple, destination addresses). It could also save short payment_id, since this is not decryptable after the fact by the sender (except if the sender is the receiver).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see what you mean. Then I guess I need to put ptx.is_onetime = is_onetime;
into the end of wallet2::transfer<T>()
and wallet2::transfer_selected()
and wallet2::transfer_selected_rct()
, correct?
BTW, overloads to wallet2::transfer()
without args cryptonote::transaction tx
and pending_tx ptx
seem to be not used anywhere. Aren't they redundant?
src/simplewallet/simplewallet.cpp
Outdated
@@ -2664,6 +2665,7 @@ bool simple_wallet::sweep_all(const std::vector<std::string> &args_) | |||
else while (!ptx_vector.empty()) | |||
{ | |||
auto & ptx = ptx_vector.back(); | |||
ptx.is_onetime = is_onetime; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same comment as above
src/wallet/wallet2.cpp
Outdated
crypto::secret_key_mult_public_key(*onetime_h, reinterpret_cast<const crypto::public_key&>(derivation), reinterpret_cast<crypto::public_key&>(derivation2)); | ||
check_acc_out_precomp(spend_public_key2, o, derivation2, i, received_bool, money_transfered, error); | ||
if (received_bool) | ||
received = 2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather see two booleans (received, and onetime, eg), makes things a lot more understandable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm surprised that boost::bind produces error when the number of function arguments is more than 9. Seriously? :/
As a workaround, I'll introduce a new struct wallet2::is_received_info
containing two bools.
src/wallet/wallet2.cpp
Outdated
// check if the following equation holds: (H(viewkey, pID) - pID) mod 256 == 0 | ||
if (pID_info.payment_id32.data[0] == pID_info.onetime_h_real.data[0]) | ||
pID_info.onetime_h = &pID_info.onetime_h_real; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does not set has_* to false if nothing found in extra
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
has_* are set to false by default, see L626
src/wallet/wallet2.cpp
Outdated
needed_fee = calculate_fee(fee_per_kb, txBlob, fee_multiplier); | ||
} while (ptx.fee < needed_fee); | ||
} while (ptx.fee < needed_fee); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't add arbitrary reformatting
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry
} | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
his should probably init to null_hash if nothing found, just to be clean
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK
} | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should also initialize to null_hash id not found
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
src/wallet/wallet2.h
Outdated
} | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
init to null_hash if nothing found
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
@@ -99,7 +99,7 @@ int main(int argc, char *argv[]) { | |||
vector<char> data; | |||
ec_scalar expected, actual; | |||
get(input, data, expected); | |||
hash_to_scalar(data.data(), data.size(), actual); | |||
::hash_to_scalar(data.data(), data.size(), actual); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did this error out if you did not add :: ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, because I defined the function crypto::hash_to_scalar()
in crypto.h and crypto-tests.h has a global scope function ::hash_to_scalar()
, so the compiler gets confused which to use due to using namespace crypto
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see it. Very strange.
src/crypto/crypto.cpp
Outdated
void crypto_ops::copy(const ec_scalar &src, ec_scalar &dst) { | ||
std::copy(src.data, src.data + 32, dst.data); | ||
} | ||
/* | ||
* generate public and secret keys from a random 256-bit integer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did this because I thought simply assigning ec_point
from one variable to another wouldn't copy the char data[32]
, but now I realized it does. So this should be unnecessary, it can be simply written as dst = src
. Correct?
I'm so used to using std::vector
or std::array
and didn't familiarize myself enough with raw arrays... (I'm surprised because raw arrays themselves cannot be copied directly.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dst = src would be fine, yes. IIRC, copying of structs/classes defaults to memcpy, which is correct in this case.
I'd be very surprised if anyone can double spend by making any changes to the code; would it be possible at all to cheat the key image checking scheme of CryptoNote? I'm just curious. |
I don't think the Cryptonote one can be cheated. Your changes adds a second key image, though, and a few places where the original key image is replaced by a second one. This is what needs looking at carefully to ensure both key images can't be used in different cases. |
I see. |
src/wallet/wallet2.cpp
Outdated
check_acc_out_precomp(spend_public_key2, o, derivation2, i, received.onetime, money_transfered, error); | ||
} | ||
} | ||
//---------------------------------------------------------------------------------------------------- | ||
static uint64_t decodeRct(const rct::rctSig & rv, const crypto::public_key pub, const crypto::secret_key &sec, unsigned int i, rct::key & mask) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor note: shouldn't the second arg be const reference as const crypto::public_key& pub
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should.
As a simple safeguard against the possible mistake of the sender not attaching the payment ID to the transaction, we could make the wallet save every payment ID issued by every |
Wow it's an ultimate super awesome feature!!! I find it as a core privacy feature, which would make monero completely anonymous. I would love to start using it as soon as possible. My c++ knowleadge is very limited. Tried to build it as per description on my ubuntu 16.04 box (amd64) and it failed with error:
I can also see that only monero-static-ubuntu-arm7 passes buildbot tests here on github pull request. Somebody please contribute and make this feature work on ubuntu-amd64? That would be a huge step forwards in terms of privacy! Thank you in advance! Really appreciate your work! |
@kenshi84 And what if somebody deletes that file by mistake? I think it would be better to prevent the user from sending this form of transaction without a payment ID |
@NanoAkron
I don't get what you mean. With |
Couldn't you enforce 'Hey user, I see you've not entered a Payment ID. This prevents us from requiring the user to store yet another file, and from debugging problems related to it in future. |
@NanoAkron
In this case, "user" refers to the sender, I suppose? |
Ah. Does the sender's wallet keep the pIDs of integrated addresses to which it sends? If it does so, could |
I came up with a way to combine disposable addresses with sub-addresses by using scalar multiplication as was used before @luigi1111's suggestion. For a sub-address
The sender's procedure of constructing a transaction is the same as the one for transferring to a sub-address where When the receiver checks if a new output key |
Ah, I remember that. What I meant is that, if reusing disposable addresses is so bad, then the wallet should save those when used, then refuse to send a second time to one that's already used. When restoring a wallet, since that list will be destroyed, one can do a similar thing by collecting all the payment ids for outgoing txes from this account (since that one is not encrypted in this case). That last one has a bit more false positive rate, but probably negligible. |
So your concern is about the sender transferring to the same disposable address more than once, correct? This scenario seems less concerning to me because IMO the use of the disposable address scheme makes sense only when the payment is really one-time and the sender doesn't know who the recipient is (e.g. ShapeShift, The other way of reuse on the recipient's side, i.e., the recipient accidentally generating an identical disposable address more than once, is unlikely to happen in practice unless the recipient is using a faulty wallet implementation with bad PRNG. So I don't really see a real need for such a reuse checking feature. What do you and other people think? |
@fluffypony Done updating MRL-0006 |
If vendors start using disposable addresses as if they are subaddresses, then this is a huge privacy problem. I suggest the wallet is modified to prevent users from paying the same disposable address twice. This would safeguard users' privacy and deter vendors from abusing disposable addresses. Imagine if someone scraped all of the disposable addresses that vendors were publishing. Then if Alice pays Bob on more than one occasion, and Bob uses different outputs received from Alice on more than one occasion to pay a vendor Charlie, then Alice can easily see that Bob is a customer of Charlie if Charlie had published a disposable address for payment by all his customers. |
I agree. So each time Bob pays to Charlie's disposable address, that address gets recorded into Bob's wallet cache file so that Bob's second attempt to pay to the same address will be detected and rejected by the wallet. This record won't be available for another wallet restored from the seed, though.
The real problem I see is that not just Alice but everybody can link all of Bob's payments to Charlie using the same disposable address due to the unencrypted payment ID. |
I see what you're saying - someone could ignore the warnings and publish a disposable address for accepting payments on the basis that they're not expecting any individual user to be hit with a warning because it may be considered unlikely any one person will attempt to pay them twice. Hopefully it will be enough of a deterrent that they will lose out if someone actually does attempt to make a second payment to them. |
I suggest the wallet is modified to prevent users from paying the same
disposable address twice. This would safeguard users' privacy and deter
vendors from abusing disposable addresses.
I agree. So each time Bob pays to Charlie's disposable address, that
address gets recorded into Bob's wallet cache file so that Bob's second
attempt to pay to the same address will be detected and rejected by the
wallet. This record won't be available for another wallet restored from the
seed, though.
Perhaps this will work in the future when file formats and such stop
changing. As it stands now, I don't have a single wallet that I've ever
maintained as a persistent entity. I.e., my wallet files get deleted,
moved, created, etc. all the time. I restore from seed *a lot*. Heck, we
even inform users to delete their wallet caches and use the .keys file to
restore their wallets if its not working well. IMO, I'm wary of things that
depend on the user maintaining things that could impact the privacy of
other members of the network.
Hopefully we stop permitting cleartext payment ID's for this exact reason.
…On Fri, Feb 10, 2017 at 8:39 AM, knaccc ***@***.***> wrote:
I see what you're saying - someone could ignore the warnings and publish a
disposable address for accepting payments on the basis that they're not
expecting any individual user to be hit with a warning because it may be
considered unlikely any one person will attempt to pay them twice.
Hopefully it will be enough of a deterrent that they will lose out if
someone actually does attempt to make a second payment to them.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#1345 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AJstjMbqSTeHzXq_XokymdiBeYXpY_koks5rbGisgaJpZM4Kzp34>
.
|
TBH it's difficult for me to see how the scenario you described would be relevant; why would a vender want to use disposable addresses to receive payments from customers, when the main purpose of the scheme is to hide who the recipient (i.e. the vendor) is from the sender (i.e. customers)? Also, even if the vendor decided for whatever reasons to receive payments by disposable addresses, assuming such addresses would be generated on the per-purchase basis like Bitcoin, why would a customer pay to the same address more than once? I really can't think of any realistic scenario. Integrated addresses would be perfectly suitable here.
First of all, such reuse would not happen normally, unless the user deliberately tries to do so. As long as the user pays to a disposable address freshly generated by the recipient, there's no worries. Second, even if the reuse did happen, for not too many times, it only means that those transactions will have the same 64bit payment ID that may suggest a link among them with certain probability, and most likely this won't affect privacy of other members in the network. Note that the space of 64bit data is not too huge and coincidental collisions do occur over a long term. Finally, although I really don't see a serious need, it'll be fairly straightforward (i.e. it's on my ToDo list) to implement a feature in the wallet to export and import various data associated with the wallet's past transactions for easy backup and maintenance, including the disposable addresses it paid to previously.
I consider deleting the wallet cache as the last resort, since the cache contains quite some precious information like tx private keys, destination addresses, tx notes, address book, etc. This is why I worked previously to replace the unportable serializer with the portable one so that the user can continue to use the same cache file after new releases and across different platforms.
You mean the 256bit long payment ID that doesn't get encrypted, right? The 64bit short payment ID needs to be permitted for this disposable address scheme to work. |
The most frequent scenario I can think of is that someone creates a web site and doesn't want their main wallet address to be listed as the donation address. They can't be bothered to create a whole new wallet so they just put a disposable address there. In other cases, I expect that people will not read the instructions, and assume that disposable addresses are just like bitcoin subaddresses, and use them in the same way without thinking. |
@moneromooo-monero |
…mplewallet to wallet2
moved again get_destination_view_key_pub tx_source_entry tx_destination_entry boost serialization from cryptonote_tx_util to cryptonote_format_util
Closing in favor of PR #1753 |
A new idea about sub-addresses was devised which also includes disposable addresses, so this PR should be halted in the meantime.
monero-project/research-lab#7
MRL-0006 draft
This is an implementation of the one-time address idea discussed earlier in Reddit. My personal motivation primarily comes from the fact that I must tell my wallet address
(A,B)
to ShapeShift every time I buy XMR there.To prevent ShapeShift from knowing the total amount of XMR I bought through them, I implemented a scheme where ShapeShift is given a one-time address
(C,D)=(hA,hB)
along with a 256bit payment IDk
whereh=hash(a,k)
. The two addresses(A,B)
and(C,D)
cannot be linked without knowing the secret viewkeya
. When a wallet (or a watch-only wallet(a,B)
) processes a new transaction having a 256bit payment ID, it performs the output key derivation also against(C,D)
to see if the fund belongs to it where the corresponding secret view & spend keys are(ha,hb)
.To reduce the additional computational cost incurred in processing new transactions, the wallet generates a one-time address with random
k
such that the following equation holds:(hash(a,k)-k) mod 256 == 0
which is found by brute force. This trick allows the wallet to ignore all payment IDs not satisfying the equation, reducing the performance impact to roughly 1/256 or 0.4% of the original cost with the naive implementation.One-time integrated address
One nice thing about the above scheme is that we don't need to ask ShapeShift to upgrade their software or anything, since the sender's procedure is exactly the same as before (i.e. transfer with 256bit payment ID). However, a downside is that it is cumbersome and error-prone to have to specify the 256bit payment ID separately.
To solve this problem, I took the same approach as the integrated address where a 64bit payment ID is integrated into the address. The integrated address itself cannot be used as-is, though, because the 64bit payment ID is stored in the transaction after being encrypted with the shared secret
rA=aR
. In our case, the sender would use the shared secretrC
which equals tohash(a,k)aR
to encryptk
; i.e., the receiver needs to knowk
in order to decrypt the encryptedk
, which is impossible. Therefore, in order for our scheme to work, the integrated 64bit payment IDk
should be stored in the transaction unencrypted. This necessitates the introduction of a new type of address similar to the integrated address, where the sender putsk
into the transaction data without encryption. When a wallet sees a new transaction with a 64bit payment ID, it treats the value as potentially the unencryptedk
and performs the additional key derivation step as described above.Two new commands
onetime_address_classic
andonetime_address
are introduced. As is the case with the current practice, it's definitely discouraged to reuse the same one-time address (i.e. the same unencrypted payment ID) which obviously leads to linkages among transactions.Example
Notice how the real address is included in the integrated address, while one-time addresses are entirely different from the real address.
Known issue (on Mac OS X 10.11.6)If you create a watch-only wallet of an address which received funds using one-time addresses and doimport_key_images
, the wallet almost always crashes with an annoying error"incorrect checksum for freed object - object was probably modified after being freed."
Funny thing is that it occasionally succeeds in importing key images (maybe 5% chance) on testnet (no hope with mainnet). I have absolutely no idea how to fix this.Bug fixed (26 Dec 2016)
ToDo list:
--
whether to abandon→ abandonedonetime_address_classic
or not--
whether to generate pIDs randomly or deterministically→ randomly--
terminology (one-time address vs disposable address vs ...)→ use disposablereplace scalar multiplication by scalar addition (suggested by Luigi)→donecold wallet & offline signing→bug fixedmechanism to prevent outputs received by one-time addresses from being spent together→ future work (seems highly related to the 'churning' concept proposed by knaccc)-- http://termbin.com/639f
make it work with RPC→ done