< vasild>
This answers the question "how much of the code modified by a given PR is covered by tests?"
< vasild>
MarcoFalke: jonatack: ^ a few days ago we discussed this. Yes, it has the deficiency that if the PR caused the coverage to drop in some file that is not modified by the PR, that will not be shown.
< vasild>
Anyway this is still useful - as long as there are bright red lines (not covered and modified by the change) this means writing more tests is warranted.
< vasild>
Or at least some extra attention during review because some of the modified code is not tested.
< bitcoin-git>
[bitcoin] jnewbery opened pull request #19178: Make mininode_lock non-reentrant (master...2020-05-mininode-lock-reentrancy) https://github.com/bitcoin/bitcoin/pull/19178
< jonatack>
vasild: nice, how did you generate it
< vasild>
jonatack: I used some python library to parse the generated HTML report and apply "opacity: 0.2;" to some lines
< vasild>
I will clean up a bit the scripts and put them on github
< luke-jr>
fanquake: #19152 might be a backport canddiate
< MarcoFalke>
vasild: Nice. The "not hit, modified" is probably the most important to look out in review
< MarcoFalke>
Happy to throw the script on DrahtBot once it is open source
< MarcoFalke>
wumpus: sipa: Would it be possible to enable cirrus ci for testing purposed on the bitcoin/bitcoin repo?
< MarcoFalke>
I am using them on my personal repos for more than a year and it works a lot nicer than travis
< MarcoFalke>
I sent an email to travis and they said that s390x and arm are "unsupported", so I think we should move on instead of throwing money at them to only reply to us that our use case is unsupported
< sipa>
MarcoFalke: happy to enable it; what does cirrus offer?
< MarcoFalke>
To get the same on travis with 2 CPU and 4 GB, we pay them thousands of dollars per year
< sipa>
heh
< sipa>
MarcoFalke: approved
< sipa>
github has pretty selective permissions apparently; this just needs read access to commits etc, and write access to the check status
< bitcoin-git>
[bitcoin] MarcoFalke closed pull request #19078: test: Add salvage test for wallet tool (master...2005-testWalletToolSalvage) https://github.com/bitcoin/bitcoin/pull/19078
< bitcoin-git>
[bitcoin] MarcoFalke reopened pull request #19078: test: Add salvage test for wallet tool (master...2005-testWalletToolSalvage) https://github.com/bitcoin/bitcoin/pull/19078
< phantomcircuit>
sipa, historically the github permissions were super vague "read" "write" glad to see that's improving
< sipa>
specifically:
< sipa>
* Read access to code
< bitcoin-git>
[bitcoin] DrahtBot closed pull request #19078: test: Add salvage test for wallet tool (master...2005-testWalletToolSalvage) https://github.com/bitcoin/bitcoin/pull/19078
< sipa>
* Read access to files located at .cirrus.yml
< sipa>
* Read access to members, metadata, and pull requests
< bitcoin-git>
[bitcoin] DrahtBot reopened pull request #19078: test: Add salvage test for wallet tool (master...2005-testWalletToolSalvage) https://github.com/bitcoin/bitcoin/pull/19078
< sipa>
* Read and write access to checks, commit statuses, and content references
< sipa>
* Write access to attach content to the following external domain: cirrus-ci.com
< phantomcircuit>
yeah that's much better than it used to be, presumably for exactly this reason
< sipa>
i'm impressed it's so fine-grained
< MarcoFalke>
sipa: thx!
< bitcoin-git>
[bitcoin] MarcoFalke opened pull request #19179: [WIP RFC DONOTMERGE] ci: Run ci configs on cirrus (master...2006-ciCirrus) https://github.com/bitcoin/bitcoin/pull/19179
< fanquake>
luke-jr: can do
< achow101>
wallet meeting?
< provoostenator>
I'm around, briefly
< achow101>
meshcollider doesn't seem to be here
< achow101>
#startmeeting
< lightningbot>
Meeting started Fri Jun 5 19:02:25 2020 UTC. The chair is achow101. Information about MeetBot at http://wiki.debian.org/MeetBot.
< sipa>
given that the same attack model (software wallet malicious, able to make the vixtim sign twice with hww) already enables the attacker to e.g. be paid twice
< bitcoin-git>
[bitcoin] hebasto opened pull request #19180: refactor: Replace RecursiveMutex with Mutex in Shutdown() (master...200605-shutdown) https://github.com/bitcoin/bitcoin/pull/19180
< achow101>
So there was the whole announcement from trezor 2 days ago about the thing in segwit where a signer could be tricked into sending money into fees
< achow101>
and they're requiring full prevtxs (i.e. non_witness_utxo) for segwit inputs
< achow101>
do we want to do the same policy to protect against that attack?
< meshcollider>
ping
< provoostenator>
A less drastic measure could be for the device to remember the last couple of inputs it signed?
< achow101>
meshcollider: pong
< achow101>
it seems that sipa doesn't think so
< sipa>
provoostenator: i'd say that's far more drastic, but it's also the only real solution
< meshcollider>
Yay it's working finally
< meshcollider>
Sorry about that
< achow101>
provoostenator: I think that would require more storage than they have
< luke-jr>
sipa: why is just including the inputs not a solution?
< sipa>
luke-jr: that doesn't prevent double paying
< provoostenator>
Not having giant PSBT files was a nice improvement...
< sipa>
luke-jr: it prevents this specific attack
< sipa>
but it doesn't prevent the victim fr being told "your signature is invalid, try again" and then just paying the attacker twice
< luke-jr>
well, that's social engineering
< achow101>
sipa: but that would be new inputs, so remembering previous inputs wouldn't matter
< luke-jr>
achow101: remembering the address could
< luke-jr>
ie, strictly forbid address reuse
< luke-jr>
but social engineers can probably work around that too
< provoostenator>
So the attack mentioned, it doesn't matter who you were paying to? Or does the attacker have to be the recipient?
< sipa>
luke-jr: right, it is - but my point is that if "attacker can convince the victim to sign twice" is part of the threat model, then this attack isn't the only problem
< luke-jr>
hmm, true
< sipa>
and this specific attack can be worked around, but others can't be with stateless HWW
< achow101>
provoostenator: it doesn't matter. the amount lost was going to fees
< provoostenator>
Right, so I don't think a spend-twice bug is fully comparable
< luke-jr>
the recipient does need to be the same address, though, right?
< gwillen>
that depends on whether the victim is looking at the address
< sipa>
for the currently-discussed attack, yes
< gwillen>
we're already positing they're signing twice even though they're only sending one transaction, so it's already got a social engineering component
< provoostenator>
If the victim doesn't look at the address, they're toast regardless.
< gwillen>
but, this is true, "oops please retry" is a much smaller social-engineering ask
< sipa>
gwillen: than what?
< provoostenator>
Once your computer has the kind of the malware that can do that, it can do so many things...
< luke-jr>
deterministic input sorting could fix this too I think?
< sipa>
luke-jr: i don't think so
< provoostenator>
It can fool your browser, fool the UI of your wallet where you "check" the address, mess with clipboard, alter the chain on disk.
< luke-jr>
provoostenator: but otoh, malware on your comnputer is what hw wallets claim to protect against
< provoostenator>
luke-jr: they do, but only to a limited extend
< provoostenator>
They can't trivially run off with your private keys
< achow101>
this attack already has a strong social engineering component in convincing the user to sign twice
< provoostenator>
But if you just let someone take over your computer long enough..
< provoostenator>
Would it help to store block height as a nonce?
< gwillen>
achow101: people are used to things like that, though, so I think most people unaware of the attack would fall for it, even if they're otherwise careful
< provoostenator>
nLockTime I mean
< gwillen>
(for example, you would have to do that if USB flaked out, probably)
< gwillen>
the coldcard has a big advantage if you're having to carry the transaction across by hand each time vs just spitting it over USB
< gwillen>
this attack really does not work in that setting
< sipa>
i think the only feasible solution is education really
< sipa>
of course fixing this specific bug is a good thing of it comes at no cost
< sipa>
but i'm unconvinced breaking "only need utxo to sign" is worth it
< achow101>
sipa: education as in educating users they should inspect their transactions before sending?
< sipa>
that they should be wary if they're told to retry signimg
< provoostenator>
For the RPC it's cheap to add an opt-in feature to fill in the UTXO for SegWit, for GUI I would find it cluttery.
< sipa>
oh we should add the full input tx where possible, i think
< sipa>
for bitcoin core this is easy to do
< achow101>
to be compatible with latest firmwares that fix this, we still need to add the full input tx
< sipa>
but i'm not sure about requiring full input tx when signing
< luke-jr>
add it, but don't require it to sign
< achow101>
this does break compatibility with previous versions of Core and HWI
< luke-jr>
is I think what sipa's saying
< achow101>
right
< sipa>
indeed
< achow101>
ack
< sipa>
unless we can get trezor to reverse their stance
< sipa>
which seems unlikely
< luke-jr>
it may be more likely than you assume
< achow101>
i believe trezor, ledger, bitbox, and coldcard have/will have the same requirement to provide the full previous tx
< luke-jr>
it's *possible* (but not certain) that the motive is to get another security fix released without drawing attention to it
< sipa>
if that happens we have no choicw to follow suit
< achow101>
although I think ledger does something where they allow single input segwit without full prevtx
< sipa>
+but
< gwillen>
achow101: I was under the impression that hww other than trezor were making this a user option at most
< achow101>
gwillen: i've been working through a bunch of trezor issues over the past couple of days, so I havnen't had the change to test out ledger's changes
< achow101>
coldcard hasn't published a new firmware yet but I'm told they probably will
< jonatack>
luke-jr: not entirely implausible given how it was handled
< achow101>
luke-jr: then they did a real poor job of it by giving users a good reason to not upgrade.
< luke-jr>
achow101: are users getting that impression?
< jonatack>
makes 2 recent trezor upgrades now that were better to avoid
< achow101>
luke-jr: yes. electrum, wasabi, and btcpay server no longer work with the new firmware. so users who want to keep using those software with their trezors are incentivized to not upgrade
< provoostenator>
What happens when you do add the full input tx, and give it to an non-upgrade hardware wallet?
< achow101>
provoostenator: usually they're fine with it. but HWI makes some assumptions about that so it makes the wrong signature
< provoostenator>
Updating HWI is probably the easiest part.
< sipa>
i have to run for a bit, will be back in 10-15 or so
< achow101>
provoostenator: you would think so....
< provoostenator>
Even if we added support for this, it'd be a while before a release / backport.
< achow101>
right
< luke-jr>
provoostenator: that's up to us
< provoostenator>
True
< achow101>
any other topics?
< achow101>
seems not
< achow101>
#endmeeting
< lightningbot>
Meeting ended Fri Jun 5 19:36:00 2020 UTC. Information about MeetBot at http://wiki.debian.org/MeetBot . (v 0.1.4)
< ja>
fjahr: are the comments for the two versions of mulnadd3 supposed to be dufferent?
< ja>
fjahr: why not bind 1103717 as a macro?
< fjahr>
ja: thanks, I will need to look into it tomorrow. Would you mind commenting in the PR? Would be great to have these conversations there so they don't get lostt :)
< sipa>
fjahr: the asm comment is wrong
< sipa>
c2 is assumed to be 0 at entry
< sipa>
not c0
< fjahr>
ok, will fix that
< sipa>
you can see that c0 is used as input and output
< sipa>
but c2 is only ever assigned to
< fjahr>
sipa: any comment on making 1103717 a macro? :)
< sipa>
fjahr: yeah that would make sense
< ja>
do you want comments on only the ASM PR? or both?
< fjahr>
ja: I can make the changes and document it as well, no worries. Just in general it would be great of have in github so it's documented :)
< luke-jr>
sipa: someone asked why "methods" shold be extensible
< promag>
re feature detection
< luke-jr>
promag: hmm
< promag>
but re getrpcwhitelist... I think we could extend to getrpccredentials that could return whitelist, blacklist, fooo...
< luke-jr>
I wonder if listfeatures (tentative new RPC name) should omit wallet-required RPCs when no wallet is specificed
< promag>
only API that I remember where I had to discover features was opengl + extensions
< promag>
also js stuff on the different browsers
< promag>
pita
< promag>
it's much easier to target a server version and stick to it.. then update client if server is updated
< luke-jr>
promag: just assuming the features are available and documenting a minimum supported server makes sense; but checking the version in software does not.
< phantomcircuit>
right so the linker is complaining about using static member functions in blockfilter.h in wallet/wallet.cpp
< phantomcircuit>
is there some magic i need to do to make that work?
< luke-jr>
phantomcircuit: GCC 10?
< phantomcircuit>
luke-jr, 8.3
< luke-jr>
oh, because it's the wallet module
< luke-jr>
wallet doesn't necessarily have block filters
< luke-jr>
gotta go through the interfaces/ stuff IIRC
< bitcoin-git>
[bitcoin] MarcoFalke opened pull request #19183: [WIP DONOTMERGE] Replace boost with C++17 (master...2005-StdVariantScriptedDiff) https://github.com/bitcoin/bitcoin/pull/19183