< phantomcircuit>
sipa, actually this should make is really easy to do parallel scans
< aj>
sipa: oops, was about to say "i'm not seeing any more premature requests either", but then realised when i picked up your updated patch i didn't reinstate the premature logging...
< aj>
hmm, and i didn't merge your patch very well anyway apparently, drat
< bitcoin-git>
[bitcoin] pstratem opened pull request #19216: wallet: Remove first parameter to ScanForWalletTransactions start_hash (master...2020-06-07-wallet-scanforwallettransactions) https://github.com/bitcoin/bitcoin/pull/19216
< phantomcircuit>
anybody know why the cirrus ci failed on #19216 ?
< gribble>
https://github.com/bitcoin/bitcoin/issues/19216 | wallet: Remove first parameter to ScanForWalletTransactions start_hash by pstratem · Pull Request #19216 · bitcoin/bitcoin · GitHub
< bitcoin-git>
[bitcoin] glowang opened pull request #19217: disambiguate block-relay-only variable names from blocksonly variables (master...2020/06/08/rename-max-block-relay-connection-name) https://github.com/bitcoin/bitcoin/pull/19217
< bitcoin-git>
[bitcoin] sipa opened pull request #19219: Replace automatic bans with discouragement filter (master...202006_discourage) https://github.com/bitcoin/bitcoin/pull/19219
< phantomcircuit>
`importwallet` doesn't update transactions that are found in the scan initiated when the wallet is loaded if they already exist, which i think could mean they get attributed to the wrong block?
< sipa>
how so?
< sipa>
you mean if there was a reorg in between?
< phantomcircuit>
sipa, i think so yes
< phantomcircuit>
if the reorg happened before you imported the wallet
< phantomcircuit>
it's the only thing that calls scan with fUpdate=false
< sipa>
but the wallet will observe the reorg when your node activates it?
< sipa>
or you mean if this happens while the wallet is not loaded?
< phantomcircuit>
sipa, if the reorg happens when you dont have the wallet activated
< phantomcircuit>
sipa, it's kind of a catch 22
< phantomcircuit>
it shouldn't be possible for the fUpdate flag to mean anything
< bitcoin-git>
[bitcoin] hebasto opened pull request #19220: refactor: Replace RecursiveMutex with Mutex in warnings.cpp (master...200609-warn-mx) https://github.com/bitcoin/bitcoin/pull/19220
< wumpus>
luke-jr: yes, I managed to do it! (was pretty simple- "gpg --cert-digest-algo sha512 --expert --edit-key", then "sign", then re-upload the key to keyservers)
< provoostenator>
sipa fjahr: do I understand correctly that MuHash behaves as a set? I.e. if you add a UTXO that already exists, you get the same hash?
< provoostenator>
If so, does that offer a memory efficient alternative to dbcache? (ignoring consensus failure risk)
< aj>
provoostenator: what? that can't be right? if it were you could test for membership easily?
< provoostenator>
You could check if an input exists in the UTXO set by addin git
< provoostenator>
aj: exactly, so it might be wrong :-)
< provoostenator>
Or it's computationally expensive, but memory efficient. Either way, I understand before reviewing #19055
< provoostenator>
Ah I guess I see the practical problem: you still need to track the spending condition, so you still need to update a database, which is faster in RAM
< provoostenator>
But maybe it allows for a different structure of the dbache.
< provoostenator>
Each item in the MuHash UTXO set is made of: outpoint + (height * 2 + isCoinbase) + (value + scriptPubKey)
< fjahr>
provoostenator: unfortunately not, you can add the same number multiple times and the hash changes. In some of the comments I think the word 'set' is misused, did that make you think that? It came up in the last review club as well, I think.
< provoostenator>
fjahr: because of the terminology in muhash.h: "Multiply (resulting in a hash for the union of the sets)"
< provoostenator>
Is it non-determistic as well?
< provoostenator>
No, that can't be, given the test vector.
< fjahr>
Yeah, I will fix that.
< fjahr>
it is deterministic.
< provoostenator>
I'll read the Review Club minutes to see if I can wrap my head around how something can allow unions, not be a set, yet still bedeterministic.
< provoostenator>
It's path dependent?
< fjahr>
No :) Did you take a look at the python implementation already? We will talk about it tomorrow. It is easier to grasp the algo with it. In the last pr review club we talked more about the project as a whole with the index, tomorrow should be more interesting for the algo.
< provoostenator>
I'll try to make tomorrows PR review club
< fjahr>
would be great :)
< provoostenator>
I already left a comment on your original PR that we should design the index such that it's easy to add more, if we want to try a different hash function.
< bitcoin-git>
bitcoin/master bacbfb6 Hennadii Stepanov: refactor: Replace RecursiveMutex with Mutex in warnings.cpp
< bitcoin-git>
bitcoin/master 9ad6f14 MarcoFalke: Merge #19220: refactor: Replace RecursiveMutex with Mutex in warnings.cpp
< aj>
fjahr: why #define's instead of inline functions in muhash.cpp? mul() sets c2 to 0 even though it doesn't take it as a param which seems pretty weird too (it's already zeroed the only place it's called, so it's technically okay, but...)
< bitcoin-git>
[bitcoin] MarcoFalke merged pull request #19220: refactor: Replace RecursiveMutex with Mutex in warnings.cpp (master...200609-warn-mx) https://github.com/bitcoin/bitcoin/pull/19220
< fjahr>
aj: a macro was suggested and I did not have too much of an opinion on it
< provoostenator>
Reading the paper it's less like a set, more like an array. Adding stuff isn't a union but a join. Removing stuff implies you can have "negative" entries in the array.
< fjahr>
aj: oh, I think I misunderstood your question. But the answer is similar. I did not change sipa's code too much unless I needed to fix something. But happy to make changes based on feedback.
< fjahr>
provoostenator: yes, it is really just like multiplication and division (over a finite field).
< provoostenator>
Maybe instead of "add to set" say "multiply numerator", and instead of "remove from set" say "multiply denominator"
< provoostenator>
The compact Python code is certainly easier to follow
< phantomcircuit>
sipa, yeah it really looks like the fUpdate flag *shouldn't* do anything, but then again the entire rescan functionality shouldn't do anything
< sipa>
fjahr: i need to check the paper again, but i'm not sure that MuHash is secure when the same element is allowed to be added multiple times (under control of an attacker)
< sipa>
fjahr: which is why it's called a set and not a multiset
< sipa>
ECMH is explicitly secure even when items have multiplicity
< sipa>
provoostenator: so it is a set, but only by virtue of not being allowed to add the same element multiple times (or not being allowed to remove something that you don't know isn't already in it)
< sipa>
not by the data structure itself enforcing that
< fjahr>
sipa: ok, that makes sense. I wasn't sure about security implication, I just knew it is mathematically possible. I will make sure to add that to the documentation.
< sipa>
fjahr: my somewhat naive intuition is that it's actually fine with multiplicity, but given that i don't remember that being proven, and we don't need, i'd rather be conservative
< phantomcircuit>
sipa, im inclined to remove the fUpdate flag with an override to always use fUpdate