< 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)...?
< 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/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>
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)
< 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
< 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.
< 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 :-)
< 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>
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?