< bitcoin-git> [bitcoin] qqz898 opened pull request #14766: 0.17 (master...0.17) https://github.com/bitcoin/bitcoin/pull/14766
< bitcoin-git> [bitcoin] qqz898 closed pull request #14766: 0.17 (master...0.17) https://github.com/bitcoin/bitcoin/pull/14766
< bitcoin-git> [bitcoin] qqz898 closed pull request #14766: 0.17 (master...0.17) https://github.com/bitcoin/bitcoin/pull/14766
< kallewoof> *headscratch* why would I get errors about "ifstream in fs not having a type" in fs.h (which I haven't touched) included from bitcoind.cpp (which I haven't touched)...?
< sipa> kallewoof: master is broken
< kallewoof> sipa: oh
< bitcoin-git> [bitcoin] kallewoof opened pull request #14768: revert removal of fstream.hpp header in fs.h (master...restore-fs-h-include-boost-fs-fstream) https://github.com/bitcoin/bitcoin/pull/14768
< bitcoin-git> [bitcoin] MeshCollider closed pull request #14763: Remove filesystem/fstream from EXPECTED_BOOST_INCLUDES (master...2018/11/expected_boost_includes) https://github.com/bitcoin/bitcoin/pull/14763
< gwillen> gmaxwell: can you explain to me what your comment on 14588 means? (I assume it's not for me, but for committers)
< sipa> gwillen: gmaxwell suggests we may want to include it in 0.17.1
< sipa> and no, not aimed at you (but feel to comment whether you think that's a good idea)
< gmaxwell> yeah, feel free to say if you think it should/shouldn't be.
< wumpus> meshcollider: I'm now
< wumpus> provoostenator: that really shouldn't have been merged before being sure about it, linters cause enough issues without feeding them false input
< wumpus> oh it doesn't change the linter config, just removes a header too much causing compile issues
< wumpus> why is fstream only needed on some platforms?
< wumpus> oh I see, it sometimes gets included through other headers
< bitcoin-git> [bitcoin] laanwj pushed 2 new commits to master: https://github.com/bitcoin/bitcoin/compare/09f1d7fe7243...1b99d153d071
< bitcoin-git> bitcoin/master e816b34 Karl-Johan Alm: revert removal of fstream.hpp header in fs.h...
< bitcoin-git> bitcoin/master 1b99d15 Wladimir J. van der Laan: Merge #14768: revert removal of fstream.hpp header in fs.h...
< bitcoin-git> [bitcoin] laanwj closed pull request #14768: revert removal of fstream.hpp header in fs.h (master...restore-fs-h-include-boost-fs-fstream) https://github.com/bitcoin/bitcoin/pull/14768
< wumpus> ouch, travis being down and it not being noticed is quite dangerous
< wumpus> luckily it was only a header issue then
< luke-jr> if Travis being down is dangerous, the real danger is the conclusion that things are being merged without real review :x
< luke-jr> ie, it's being used as more than just a mere convenience
< wumpus> nah not fatally dangerous, the PR didn't go without review
< promag> is there a way to make a long RPC call? like server side sleep?
< wumpus> promag: yes, RPC calls can sleep as long as they want, although the client has to make sure they don't time out
< promag> I thought we were doing blind merges :D
< wumpus> it does hold up a RPC thread but there's other calls that can take a long time for example the utxo stats one
< promag> wumpus: for testing I don't think we have?
< wumpus> I think the mining RPC does long-polling
< wumpus> and there's RPCs to wait for next block and such?
< promag> ah!
< promag> thank you sir
< wumpus> np
< promag> do we have tests to concurrently call RPC to the same node?
< wumpus> promag: I don't think so
< provoostenator> One reason we didn't notice Travis missing is that there's still a green checkbox from the other tools. That's strange, shouldn't Github show an error if any integration is missing / fails?
< wumpus> normally yes
< Varunram> only if it misses / fails though, not if it doesn't boot up the tests in the first place
< wumpus> yes, that sound plausible
< bitcoin-git> [bitcoin] laanwj pushed 4 new commits to master: https://github.com/bitcoin/bitcoin/compare/1b99d153d071...afa506f6ebeb
< bitcoin-git> bitcoin/master c456fbd Russell Yanofsky: Refactor: Move m_db pointers into BerkeleyDatabase...
< bitcoin-git> bitcoin/master 15c93f0 Chun Kuan Lee: wallet: Add trailing wallet.dat when detecting duplicate wallet if it's a directory.
< bitcoin-git> bitcoin/master 5912031 Chun Kuan Lee: wallet: Create IsDatabaseLoaded function
< bitcoin-git> [bitcoin] laanwj closed pull request #14552: wallet: detecting duplicate wallet by comparing the db filename. (master...default-wallet-fix) https://github.com/bitcoin/bitcoin/pull/14552
< bitcoin-git> [bitcoin] scravy opened pull request #14770: Do not specify sudo in .travis (master...patch-1) https://github.com/bitcoin/bitcoin/pull/14770
< wumpus> promag: huh didn't see that, you had already utACKed it
< promag> yeah, that was a previous commit
< wumpus> *sigh* ok
< promag> how can I cancel an utACK?
< wumpus> edit->delete it
< promag> anyway, ken2812221_ ^
< wumpus> I can revert it if that's really necessary
< promag> I don't think so
< promag> the mutex is for g_dbenvs, but there is no mutex for WalletEnvironment
< promag> so I wonder if it should use the same
< LGzr> Hola, all. I've a question about the p2sh example at https://bitcoin.org/en/developer-examples#p2sh-multisig; is there a more appropriate forum than here? (Question is: what purpose it serves that createmultisig uses public key for one, and addresses for the others)
< wumpus> LGzr: I think https://bitcoin.stackexchange.com/ is the best place to ask such things
< LGzr> Wumpus - thanks!
< achow101> meshcollider: can you rebase #14491
< gribble> https://github.com/bitcoin/bitcoin/issues/14491 | Allow descriptor imports with importmulti by MeshCollider · Pull Request #14491 · bitcoin/bitcoin · GitHub
< bitcoin-git> [bitcoin] MarcoFalke closed pull request #14755: Remove redundant readme info in /doc (master...issue-14639) https://github.com/bitcoin/bitcoin/pull/14755
< provoostenator> I notice that dumpwallet (on a fresh wallet) contains two sections, I'm confused why:
< provoostenator> cV6... 2018-11-20T17:23:30Z reserve=1 # addr=tb1q... hdkeypath=m/0'/0'/11'
< provoostenator> 001433... 0 script=1 # addr=2Ms...
< sipa> the script is the p2sh wrapper of that address, i suppose
< provoostenator> Ah hence they start with 0014? So the first section is for legacy & native segwit, the second for p2sh wrapped segwit? But why don't the p2sh entries show an hdkeypath?
< sipa> they're not keys
< sipa> in the current wallet design, you don't import addresses
< sipa> you import keys and scripts, and it figures out on its own what that implies for what is yours
< sipa> in practice for the P2SH-P2WPKH address derived from a wallet key, IsMine works as follows:
< sipa> * it sees a P2SH address, so it looks up the script for that scripthash
< sipa> * it notices the script is a P2WPKH script, so it recurses by replacing it with the corresponding P2PKH script
< sipa> * it notices it's a P2PKH script, so it looks up the pubkeyhash
< sipa> * it finds the private key for it, so it's considered spendable
< sipa> but the only thing in that whole sequence that has a keypath is the key at the end
< bitcoin-git> [bitcoin] laanwj pushed 2 new commits to master: https://github.com/bitcoin/bitcoin/compare/afa506f6ebeb...6b90a2a0e065
< bitcoin-git> bitcoin/master feeef7d Julian Fleischer: Do not specify sudo in .travis...
< bitcoin-git> bitcoin/master 6b90a2a Wladimir J. van der Laan: Merge #14770: travis: Do not specify sudo in .travis...
< bitcoin-git> [bitcoin] laanwj closed pull request #14770: travis: Do not specify sudo in .travis (master...patch-1) https://github.com/bitcoin/bitcoin/pull/14770
< provoostenator> sipa: thanks. Perhaps no point in cleaning up the current wallet, but dumpwallet could add comments to the script entries like "p2sh-segwit script for hdkeypath=m/0..."?
< sipa> provoostenator: or we could dump the inferred descriptor for each entry :)
< provoostenator> That makes sense on top of #11803, but more for the key entries than for the script entries.
< gribble> https://github.com/bitcoin/bitcoin/issues/11803 | Bugfix: RPC/Wallet: Include HD key metadata in dumpwallet by luke-jr · Pull Request #11803 · bitcoin/bitcoin · GitHub
< sipa> provoostenator: no, that makes no sense
< sipa> keys are keys, not addresses
< provoostenator> But they're supposed to be interpreted as combo() descriptors
< sipa> they participate in all scripts that use them
< provoostenator> Depending on wallet age
< sipa> and we can't even efficiently iterate those
< sipa> yes, they also (for now) imply a combo descriptor on their own
< sipa> but that's not very informative
< sipa> "this key acts in all the way it itself can be an address!"
< provoostenator> It's perhaps more informative than the current address= comment which is misleading.
< sipa> ah, i see
< sipa> it could list all addresses, or a combo descriptor - that's fair
< sipa> but for scripts it's actually interesting, as it will tell which which keys it is related to
< provoostenator> Yes, but the script is only relevant for p2sh wrapped segwit, right? (in a default new wallet)
< sipa> sure
< sipa> or anything you manually imported
< sipa> (multisig etc)
< provoostenator> In that case I think it's more clear for the comment to indicate that it's only there for p2sh wrapped segwit support, i.e. those entries are redundant once descriptors take over.
< sipa> dumpwallet as a whole will need to be revamped post-descriptors
< sipa> i don't understand what you're trying to argue for
< sipa> scripts are not just there for p2sh wrapped segwit
< sipa> they are there for p2sh and p2wsh everything
< provoostenator> Indeed, but it might be useful to get it good enough so it's easier to test things (like upgrade paths) once we have descriptor functionality complete.
< provoostenator> Right now it's really to inspect what is actually in a wallet.
< provoostenator> *really hard
< provoostenator> The comment would only be there for scripts that relate to the standard wallet, not any imported / generated p2(w)sh stuff.
< sipa> we don't know that
< sipa> there is just a bunch of scripts
< sipa> they're not in any way linked to other wallet information
< provoostenator> You could re-match them using the private keys from all keys with an hdmasterkeyid?
< sipa> or you could just print their inferred descriptor
< sipa> which works for everything
< sipa> and is efficient
< sipa> even that may be confusing, though
< sipa> as a p2sh-p2wsh script will have two entries
< sipa> and we don't dump watched scripts
< * sipa> would like to see dumpwallet die
< provoostenator> I think dumpwallet can be useful for testing transition to descriptor based wallets, but I'm fine with making it die after that :-)
< sipa> yeah, of course
< provoostenator> Look for my example "do some wallet operation on wallet"
< provoostenator> It passes an Authorization: Basic param with the request, which contains the RPC username and password in some base64 converted form. Afaik it has full RPC access.
< provoostenator> That's not because of this PR.
< provoostenator> (or if it is, that's very underhanded code)
< sipa> provoostenator: yes, i see that, but that is just the RPC interface
< provoostenator> Oh wait, but then what is the -rest parameter doing?
< sipa> not the REST interface, which uses .../rest/... URLs
< provoostenator> -rpcport=8080 is just cancelling my -rest=1?
< sipa> no
< sipa> -rest enables the REST interface
< sipa> that doesn't mean you can't use RPC anymore
< sipa> ah, -rpcport is somewhat confusingly named i guess, as that option indeed does affect both rpc and rest
< provoostenator> Ah, it seems very unhealthy that RPC and REST are so intertwined.
< provoostenator> Shouldn't REST just launch it's own server?
< sipa> why?
< sipa> they're entirely separate, apart from both being HTTP
< sipa> rpcport should be named httpport i guess, but it's a bit late for that
< provoostenator> I might not want to expose my RPC port to the internet
< provoostenator> Oh wait, it doesn't move the RPC port? Ok, that is _really_ confusing :-)
< sipa> sigh
< sipa> rpcport sets the HTTP listen port
< sipa> both RPC and REST use HTTP
< sipa> and you shouldn't expose either to the internet
< sipa> only the P2P interface is designed to be DoS resilient
< provoostenator> Ok, but DoS is a different threat than someone gaining RPC control.
< sipa> sure
< sipa> but neither are designed to be exposed to the internet
< wumpus> it would be fully possible to start multiple http servers, and have rpc run on one and rest on the other, though I doubt it's going to be a feature that sees much use
< provoostenator> I think it would be less confusing if it was a seperate server with its own port, but for this ticket that doesn't matter. The trick is just to only add CORS stuff to /rest/* so we don't have to worry about the phishing scenario I described.
< wumpus> *if* you want to expose anything to the internet you're going to want to put an nginx server or such in front, which would filter what URLs are allowed
< wumpus> I don't see how it's confusing, it has been like this since the beginning and AFAIK it's all documented *shrugs*
< sipa> provoostenator: why do you need CORS on REST?
< wumpus> that sounds really scary
< provoostenator> Well that's the point of that PR, which we could debate in general.
< sipa> i thought it was about something something browser security (i don't understand browsers, this may be my misunderstanding)
< sipa> but for REST there is no security needed
< provoostenator> At the moment browsers refuse to connect to the REST RPC via JSON.
< wumpus> it'd be about letting websites make requests to the local bitcoind instance, hence 'cross-origin'
< wumpus> I think that's completely ill-advices
< sipa> ah, it's permitting something, not denying something
< wumpus> right
< provoostenator> Yes
< sipa> i see
< provoostenator> I don't see any downside with read-only access.
< sipa> permitting sites to access REST sounds much less scary than letting them access RPC though
< wumpus> it increases the attack surface to any random website
< provoostenator> That's true.
< wumpus> what if it's not really read-only but there is some bug in the REST code or a heartbleed-kind of bug that exposes memory and private keys or whoknowswhat
< wumpus> I really prefer not having that kind of stuff in bitcoind, sorry
< provoostenator> I guess that begs the question why this REST service exists in the first place.
< wumpus> it's a fast way of accessing bitcoind's state
< wumpus> no need for JSON parsing and formatting, it can give various binary objects directly
< wumpus> also it can work without autentication for *local* programs, which is useful
< jarthur> Yea, and that's a big plus. In sidecar services that need to parse the blockchain from a node, so much CPU time is spent on JSON parsing when using the RPC api.
< wumpus> that doesn't mean you'd want to either expose it to the whole internet, or to every website in a browser
< wumpus> that'd instantly lift any vulnerability from local-only and limited scope to, pretty much, rce
< wumpus> same for DoS
< provoostenator> So it's for fast queries by semi-trusted software.
< wumpus> yes
< provoostenator> That makes sense. It might be good to document that intention to set expectations: https://github.com/bitcoin/bitcoin/blob/master/doc/REST-interface.md
< wumpus> or say, at most internal to your company if you bind on a local network interface and know what you're doing
< wumpus> bitcoind's P2P interface is the only interface that is hardened for public internet access, and even there improvements are certainly possible
< sipa> provoostenator: that sounds like a good idea
< wumpus> yes
< wumpus> would be good to document that better
< sipa> wumpus: so, in fairness, the issue of exposing REST to browsers (or any local program running untrusted code) already exists, and it's only through browsers' self-restraint that this is somewhat curbed
< sipa> i don't think CORS makes things any worse, it's just a question of whether we should treat browsers accessing REST as a supported/recommended practice
< sipa> and in general i think the answer is no, but perhaps for a company's internal site it makes sense
< gmaxwell> at least so long as REST is merely a DOS/RCE vector and not intentionally given access to anything too interesting it's not as big a deal.
< gmaxwell> don't want to repeat ethereum's mistakes.
< wumpus> sipa: *everything* is due to browser's self restraint, any security at all, if javascript would run unrestricted it would be really bad
< wumpus> and sure, there could be an option to add CORS headers for specific sites
< sipa> the PR lets you configure domains
< wumpus> if anyone is going to use that and test that
< wumpus> I'm just afraid of scope creep, as you know
< sipa> wumpus: i understand, but my point is that this isn't really restricted to browsers; the issue exists equally for say someone running an untrusted VM image that makes a connection to the host network
< wumpus> look, there's this concern we haven't even ever *thought* about and suddenly it needs to be considered in bitcoin core
< sipa> so our job should be to make sure that by default, these things aren't a concern, and document them well
< wumpus> yes, okay, the general case where peopel run untrusted software that can access local network, it's not possible to protect against that
< provoostenator> Someone who really wants this could add an nginx server that just adds the header etc.
< sipa> (i'm not arguing in favor of the CORS PR; I don't understand the use cases well enough, it just sounds to me that if remote access from local software is a problem, the solution is elsewhere)
< wumpus> remote access from random websites that the user opens in their browser is a problem
< wumpus> 'remote' I mean, it's effectively local, the browser protects that using CORS
< sipa> i mean things like by default not exposing REST, not exposing RPC port publicly, not having a trivial RPC username/password are all protections against this
< provoostenator> Scope creep is a nice thing to prevent. Even having to explain in documentation what the pitfalls of CORS are, and keep track of that, is overhead.
< gmaxwell> sipa: to be fair 'VM sandbox that has network access as localhost' is kind of a special case that applies to FAR fewer users than "I run a browser"
< sipa> wumpus: also, unix domain sockets instead of TCP/IP for RPC would be an improvement
< sipa> so it's at least restricted to the same user on the same machine
< jarthur> Yea, someone just needs to find time to finish that out.
< jarthur> Maybe I'll get some time this weekend if no one else is working on it
< wumpus> yes unix domain sockets would be great, I should rebase that some time
< wumpus> or someone else :)
< provoostenator> Unix domain sockets also to get data in other formats than JSON (so you don't need the REST API at all)?
< wumpus> it'd be the same http server just over a different socket
< wumpus> not a completely different API
< bitcoin-git> [bitcoin] vim88 closed pull request #14753: Refactor: Changes postincrement to preincrement for iterator in for loops in src/wallet files (master...postincrement_to_preincrement_src_wallet) https://github.com/bitcoin/bitcoin/pull/14753
< jnewbery> promag: > do we have tests to concurrently call RPC to the same node?
< jnewbery> yes: check mining_getblocktemplate_longpoll.py
< promag> jnewbery: hi, yap saw that
< promag> see #14670
< gribble> https://github.com/bitcoin/bitcoin/issues/14670 | http: Fix HTTP server shutdown by promag · Pull Request #14670 · bitcoin/bitcoin · GitHub
< jnewbery> If you want to do similar in a new test, I recommend you take the LongPoll thread class and lift it into TestNode
< promag> leave a comment there please, bbl
< bitcoin-git> [bitcoin] MarcoFalke opened pull request #14771: test: Add BOOST_REQUIRE to getters returning optional (master...Mf1811-testNoDiscard) https://github.com/bitcoin/bitcoin/pull/14771