< bitcoin-git> [bitcoin] fanquake pushed 4 commits to master: https://github.com/bitcoin/bitcoin/compare/01b45b2e016f...4ede05d421e7
< bitcoin-git> bitcoin/master faa958b MarcoFalke: txindex: Remove unused boost/thread
< bitcoin-git> bitcoin/master fad8c89 MarcoFalke: txdb: Remove unused boost/thread
< bitcoin-git> bitcoin/master 89f9fef Hennadii Stepanov: refactor: Specify boost/thread/thread.hpp explicitly
< bitcoin-git> [bitcoin] fanquake merged pull request #18758: Remove unused boost/thread (master...2004-noBoostThread) https://github.com/bitcoin/bitcoin/pull/18758
< MM77788811> What's the minimum gcc/clang version to build bitcoin? I didn't find this info in the repo.
< sipa> gcc 4.8, clang 3.3
< fanquake> Note that it's actually 5.1 if you're doing a Windows cross compile
< fanquake> Although this will be irrelevant once we require c++17
< MM77788811> Thanks sipa
< MM77788811> When will we allow to use C++17 features?
< sipa> optional ones in 0.21
< sipa> relying on c++17 features in 0.22
< fanquake> Discussion in #16684
< gribble> https://github.com/bitcoin/bitcoin/issues/16684 | Discussion: upgrading to C++17 · Issue #16684 · bitcoin/bitcoin · GitHub
< sipa> (e.g. the fuzzers currently already require c++17)
< sipa> the current codebase is compiled and tested on both c++11 and c++17
< MM77788811> awesome! Thanks for the link gribble, I'm going to give it a read.
< sipa> gribble is a bot ;)
< aj> MM77788811 probably just doesn't want to be first against the wall come the robot revolution
< MM77788811> 😅
< sipa> ah, the marketing department of the sirius cybernetics corportion
< MM77788811> I'm really polite to my Alexa too, thank her whenever she did something right.
< bitcoin-git> [bitcoin] hebasto opened pull request #19172: test: Do not swallow flake8 exit code (master...200605-mypy) https://github.com/bitcoin/bitcoin/pull/19172
< bitcoin-git> [bitcoin] jonasschnelli closed pull request #13389: Utils and libraries: Fix #13371 - move umask operation earlier in AppInit() (master...13371) https://github.com/bitcoin/bitcoin/pull/13389
< bitcoin-git> [bitcoin] jonasschnelli pushed 3 commits to master: https://github.com/bitcoin/bitcoin/compare/4ede05d421e7...7f9800caf90d
< bitcoin-git> bitcoin/master f30960a João Barbosa: gui: Add closeAllWallets to WalletController
< bitcoin-git> bitcoin/master c4b5748 João Barbosa: gui: Add Close All Wallets action
< bitcoin-git> bitcoin/master 7f9800c Jonas Schnelli: Merge #15202: gui: Add Close All Wallets action
< bitcoin-git> [bitcoin] jonasschnelli merged pull request #15202: gui: Add Close All Wallets action (master...2019-01-closeallwallets) https://github.com/bitcoin/bitcoin/pull/15202
< bitcoin-git> [bitcoin] jonasschnelli pushed 2 commits to master: https://github.com/bitcoin/bitcoin/compare/7f9800caf90d...f4f222045693
< bitcoin-git> bitcoin/master f46b678 Vasil Dimov: qt: lock cs_main, m_cached_tip_mutex in that order
< bitcoin-git> bitcoin/master f4f2220 Jonas Schnelli: Merge #19132: qt: lock cs_main, m_cached_tip_mutex in that order
< bitcoin-git> [bitcoin] jonasschnelli merged pull request #19132: qt: lock cs_main, m_cached_tip_mutex in that order (master...lock_order_m_cached_tip_mutex) https://github.com/bitcoin/bitcoin/pull/19132
< bitcoin-git> [bitcoin] fanquake pushed 2 commits to master: https://github.com/bitcoin/bitcoin/compare/f4f222045693...b55b5b6c3d93
< bitcoin-git> bitcoin/master fa7e002 MarcoFalke: ci: tsan with wallet
< bitcoin-git> bitcoin/master b55b5b6 fanquake: Merge #19164: ci: tsan with wallet
< bitcoin-git> [bitcoin] fanquake merged pull request #19164: ci: tsan with wallet (master...2006-ciTsanWallet) https://github.com/bitcoin/bitcoin/pull/19164
< bitcoin-git> [bitcoin] vasild opened pull request #19173: build: turn on --enable-c++17 by --enable-fuzz (master...enable_c++17_by_fuzz) https://github.com/bitcoin/bitcoin/pull/19173
< bitcoin-git> [bitcoin] MarcoFalke pushed 2 commits to master: https://github.com/bitcoin/bitcoin/compare/b55b5b6c3d93...aa35ea55021d
< bitcoin-git> bitcoin/master 0012471 Vasil Dimov: build: turn on --enable-c++17 by --enable-fuzz
< bitcoin-git> bitcoin/master aa35ea5 MarcoFalke: Merge #19173: build: turn on --enable-c++17 by --enable-fuzz
< bitcoin-git> [bitcoin] MarcoFalke merged pull request #19173: build: turn on --enable-c++17 by --enable-fuzz (master...enable_c++17_by_fuzz) https://github.com/bitcoin/bitcoin/pull/19173
< bitcoin-git> [bitcoin] theStack opened pull request #19174: refactor: replace CConnman/BanMan pointers by references in net_processing.cpp (master...20200602-refactor-use-cconnman-references-within-net_processing) https://github.com/bitcoin/bitcoin/pull/19174
< bitcoin-git> [bitcoin] MarcoFalke pushed 3 commits to master: https://github.com/bitcoin/bitcoin/compare/aa35ea55021d...0fc6ea216c00
< bitcoin-git> bitcoin/master e783197 Russell Yanofsky: refactor: replace RegisterWalletRPCCommands with GetWalletRPCCommands
< bitcoin-git> bitcoin/master 4a7253a Russell Yanofsky: Remove g_rpc_chain global
< bitcoin-git> bitcoin/master 0fc6ea2 MarcoFalke: Merge #19096: Remove g_rpc_chain global
< bitcoin-git> [bitcoin] MarcoFalke merged pull request #19096: Remove g_rpc_chain global (master...pr/wc) https://github.com/bitcoin/bitcoin/pull/19096
< bitcoin-git> [bitcoin] laanwj opened pull request #19176: refactor: Error message bilingual_str consistency (master...2020_06_bilingual_str) https://github.com/bitcoin/bitcoin/pull/19176
< wumpus> it was a pretty nifty idea to make _() return a struct with both the original message and the translated one, good use of type safety
< bitcoin-git> [bitcoin] troygiorshev opened pull request #19177: p2p: Fix and clean p2p_invalid_messages functional tests (master...p2p-refactor-fix-tests) https://github.com/bitcoin/bitcoin/pull/19177
< vasild> I prepared a code coverage report for a PR, highlighting the lines that were modifeid by the PR: https://people.freebsd.org/~vd/pr19031_coverage_of_modified_code/src/netaddress.cpp.gcov.html#153 (or rather, the lines that are not touched by the change are dimmed).
< gribble> https://github.com/bitcoin/bitcoin/issues/153 | rfc1123Time locale fix. by gavinandresen · Pull Request #153 · bitcoin/bitcoin · GitHub
< 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
< gribble> https://github.com/bitcoin/bitcoin/issues/19152 | build: improve build OS configure output by skmcontrib · Pull Request #19152 · bitcoin/bitcoin · GitHub
< 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> 10 parallel builds for free
< MarcoFalke> Up to 8 cores, 8GB of RAM
< 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.
< lightningbot> Useful Commands: #action #agreed #help #info #idea #link #topic.
< achow101> any topics?
< provoostenator> HWW?
< achow101> one thing I would like to discuss is whether we should change our signer policy for PSBTs
< jonatack> hi
< provoostenator> I haven't done much other than rebase, but happy to answer questions
< provoostenator> Oh yeah, that's a good one.
< luke-jr> achow101: presumably with a synced node, we can access the full input tx?
< achow101> #bitcoin-core-dev Wallet Meeting: wumpus sipa gmaxwell jonasschnelli morcos luke-jr sdaftuar jtimon cfields petertodd kanzure bluematt instagibbs phantomcircuit codeshark michagogo marcofalke paveljanik NicolasDorier jl2012 achow101 meshcollider jnewbery maaku fanquake promag provoostenator aj Chris_Stewart_5 dongcarl gwillen jamesob ken281221 ryanofsky gleb moneyball kvaciral ariard digi_james amiti fjahr jeremyrubin lightlike
< achow101> emilengler jonatack hebasto jb55 elichai2
< fjahr> hi
< sipa> having thought about it more, i'm not sure it's worth requiring full input txn
< achow101> #topic HWW (provoostenator)
< achow101> any updates on that?
< achow101> FYI meshcollider broke is IRC and can't send messages
< provoostenator> The first PR for that is #15382
< gribble> https://github.com/bitcoin/bitcoin/issues/15382 | util: add runCommandParseJSON by Sjors · Pull Request #15382 · bitcoin/bitcoin · GitHub
< 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
< provoostenator> And the meat is in #16546
< gribble> https://github.com/bitcoin/bitcoin/issues/16546 | External signer support - Wallet Box edition by Sjors · Pull Request #16546 · bitcoin/bitcoin · GitHub
< achow101> so just review those?
< provoostenator> achow101: yup
< provoostenator> At least as a start.
< achow101> cool
< achow101> anything you wanted to discuss right now?
< provoostenator> There's also some dependencies
< provoostenator> But I think those two PR's can be reviewed mostly without worrying about these other dependencies. Just go for them if you're bored.
< provoostenator> Nothing specific to discuss.
< phantomcircuit> hi
< achow101> alright
< achow101> #topic Changing PSBT signer policy (achow101)
< 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)
< meshcollider> Thanks achow101
< bitcoin-git> [bitcoin] fjahr opened pull request #19181: Add ASM optimizations for MuHash3072 (master...csi-4-muhash-asm) https://github.com/bitcoin/bitcoin/pull/19181
< 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 :)
< fjahr> ja: thanks for reviewing!
< promag> > Knots for example has many RPCs before they're available in Core
< promag> so?
< promag> you mean that the client should be unaware if the server is core or knots?
< luke-jr> promag: I mean it shouldn't artificially fail because of "too old version" if the server actually supports the RPCs it needs
< luke-jr> Knots is just one example
< luke-jr> I could very well see someone using stable/older bitcoind backporting specific features they need
< promag> for any specific version (core@x.x.x for instance) you know what is supported
< luke-jr> promag: not usually
< promag> no?
< luke-jr> promag: the client author would need to study each RPC server version/variant, and can never account for private backports
< promag> but how does feature discovery help there?
< promag> only difference is requesting that info from the server VS having that info on client side based on server version
< luke-jr> promag: if you backport foobar method, help() and potentially getrpcwhitelist() will tell you foobar is supported
< promag> the client should know that if he updates server from 1.1.2 to 1.1.3 then foobar is available
< luke-jr> no, it shouldn't
< luke-jr> and someone might have 1.1.2 with foobar
< promag> luke-jr: how?
< luke-jr> backporting it
< promag> custom build?
< luke-jr> yes
< promag> oh!!!!
< promag> well in that case HE knows what's available, it's his own version X)
< luke-jr> he shouldn't need to modify whatever client he's using
< luke-jr> client software should just work if the feature is available
< promag> ok, but you think that info should provided along with the rpc whitelist ?
< sipa> i don't understand what prompted this discussion
< sipa> Kixunil was completely confused about what the PR was about
< luke-jr> promag: MarcoFalke's argument is that help() works already
< luke-jr> sipa: Kixunil?
< promag> sipa: right
< luke-jr> promag: using getrpcwhitelist would avoid whitelist-triggered errors
< sipa> luke-jr: you're responding to promag, who was responding to Kixunil on the PR
< luke-jr> promag: ie, using help() means clients need to check *both* help AND rpcwhitelist
< promag> luke-jr: let me find one issue with a problem with that approach
< luke-jr> sipa: Kixunil's idea makes sense to me
< luke-jr> perhaps the method name should be changed :P
< sipa> ok, but it seems unrelated to this PR
< sipa> versioning RPCs seems like a very generic topic
< 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