< jonasschnelli>
wumpus: you mean the comment is wrong?
< wumpus>
no, I think it's correct, that's why I'm worried
< wumpus>
if you can convince me it's wrong that would be great
< wumpus>
:)
< jonasschnelli>
okay... let me see
< wumpus>
but unconditional lock-taking on cs_main should be avoided as much as possible in the GUI thread
< wumpus>
and the timer isn't some separate thread, and neither is the signal handler for updateBalance
< jonasschnelli>
wumpus: the PR 10251 would remove the timer... and..
< jonasschnelli>
wumpus: the wallet would push balance updates to the GUI
< wumpus>
so where would the balance computation happen?
< wumpus>
functions like GetUnconfirmedWatchOnlyBalance are called from the GUI thread
< wumpus>
so if the balance is dirty, the computation happens in the GUI thread, after aquiring cs_main and cs_wallet unconditionally
< jonasschnelli>
Hmm... yes. I see
< wumpus>
up to six times - for every type of balance, this could take ~6 minutes on a slower machine during heavy lock contention
< jonasschnelli>
But only if a tx gets added or block connected/disconnected, right?
< wumpus>
yes, which is very often
< wumpus>
ideally the GUI thread would *never* hang itself based on notifications
< jonasschnelli>
You think the TRY_LOCK in a poll routine performs better?
< wumpus>
this is currently the case for that timer
< jonasschnelli>
Yeah.. it should not be in the GUI thread
< wumpus>
it optimizes for a different user experience - it's slower, but it never hangs the GUI thread
< wumpus>
it wouldn't be a big deal if wallet functions didn't need cs_main
< wumpus>
taking the wallet lock itself is ok
< wumpus>
but the cs_main lock is terrible, never grab it from the GUI thread if possible, and if you need to, and it's just to update information, use TRY_LOCK
< jonasschnelli>
That was my main intention (remove cs_main from GUI), but I guess I have made it worse
< jonasschnelli>
I thought pushing would perform better then constant polling (even with a TRY_LOCK)
< jonasschnelli>
Because, if you can LOCK in a TRY_LOCK, I would have expected to also block further calls that want to aquire the LOCK during the time you calculate the balances
< bitcoin-git>
[bitcoin] luke-jr opened pull request #10595: Bugfix: RPC/Mining: Use pre-segwit sigops and limits, when working with non-segwit GBT clients (master...gbt_nosegwit_fix) https://github.com/bitcoin/bitcoin/pull/10595
< wumpus>
(for user-initiated things such as sending a transaction, it's more acceptable, though there too I'd prefer a solution that doesn't block the GUI thread)
< jonasschnelli>
Can we recalculate the balance during MarkDirty (connect/disconnect block, etc.)?
< wumpus>
the whole point of the dirty stuff is not to do that
< wumpus>
e.g.: if you're going to recalculate the balance *every* time it gets dirty, you don't need a cache
< jonasschnelli>
The idea behind the dirty/caching is to no recalculate it at every call,.. right?
< wumpus>
the markdirty is lazy evaluation: we know that the balance might have changed, but computing it can be expensive, so only compute it when necessary
< jonasschnelli>
Ideally the balance calls return the cache and the calls that have the power to invalidate the cache could recalculate?
< jonasschnelli>
I see..
< jonasschnelli>
[..] only compute it when necessary <-- make sense
< jonasschnelli>
the PR does that,.. but on the GUI thread
< luke-jr>
wumpus: I may not be able to make the meeting, but FWIW I am unsure if 10595 should be a blocker for v0.14.2 or not
< jonasschnelli>
wumpus: what about calling the getBalance() calls within a QThread and Q_EMIT balanceChanged once it's done?
< wumpus>
luke-jr: if we're unsure, it probably shouldn't be
< wumpus>
jonasschnelli: but then we're adding a thread
< jonasschnelli>
or use CScheduler?
< wumpus>
jonasschnelli: I do think eventually that's a better solution though: have a thread to communicate with the core
< wumpus>
never block in the GUI thread, have the GUI send commands/receive notifications from that thread
< jonasschnelli>
Yes. Some calls are synchronous though,.. they need redesign. But that must be done at some point
< wumpus>
but not a thread especially for updating balances, that's overkill, the current solution works ok
< jonasschnelli>
I think a tool where we can see what lock gets triggered how much and how long it has spent time in there would be of great value for the GUI
< wumpus>
the poll timer also offers congestion control: if a lot of transactions come in, it won't recompute the balance every time
< jonasschnelli>
wumpus: thanks for having a look into this and it seems to have be a waste of time... let me work towards the GUI<->Core communication thread
< wumpus>
jonasschnelli: yes, sorry for being encouraging at first then backpedalling, but I hadn't seen this
< jonasschnelli>
Yes. Great review. I'm happy you brought that up
< gmaxwell>
luke-jr: the gbt thing? I don't see how its a potential issue unless you are adding _more_ transactions to your block than the template gave you, which I doubt anyone does right now.
< luke-jr>
gmaxwell: or a lot of sigops in the generation tx possibly, but a very unlikely case
< luke-jr>
not sure even Eligius or p2pool would hit it
< luke-jr>
yeah, seems like it shouldn't be a practical problem thinking about it more
< wumpus>
jonasschnelli: and in general design direction you're right - direct notification is better than polling, but it works best if all the information can be passed in through the signal so that there's no need for a round-trip
< wumpus>
jonasschnelli: in this case that won't work due to lazy evaluation
< bitcoin-git>
[bitcoin] practicalswift opened pull request #10597: scripted-diff: Make use of C++11:s improved handling of two consecutive right angle brackets (master...right-angle-brackets) https://github.com/bitcoin/bitcoin/pull/10597
< bitcoin-git>
[bitcoin] laanwj closed pull request #10588: doc: Note preexisting bug in display of fee calculation in coin control (0.14...notebug) https://github.com/bitcoin/bitcoin/pull/10588
< bitcoin-git>
bitcoin/master e9cd778 Alex Morcos: Pass in smart fee slider value to coin control dialog...
< bitcoin-git>
bitcoin/master 7c72fb9 Wladimir J. van der Laan: Merge #10582: Pass in smart fee slider value to coin control dialog...
< bitcoin-git>
[bitcoin] laanwj closed pull request #10582: Pass in smart fee slider value to coin control dialog (master...fixcoincontrolfee) https://github.com/bitcoin/bitcoin/pull/10582
< ryanofsky>
wumpus, jonasschnelli: related to this topic, not sure if you saw #10504 (also had a question for you in #10244)
< bitcoin-git>
bitcoin/master 1bebfc8 Alex Morcos: Output Fee Estimation Calculations in CreateTransaction
< bitcoin-git>
bitcoin/master c2ab38b Wladimir J. van der Laan: Merge #10284: Always log debug information for fee calculation in CreateTransaction...
< bitcoin-git>
[bitcoin] laanwj closed pull request #10284: Always log debug information for fee calculation in CreateTransaction (master...debugEstimates) https://github.com/bitcoin/bitcoin/pull/10284
< bitcoin-git>
[bitcoin] paveljanik opened pull request #10598: Supress struct/class mismatch warnings introduced in #10284 (master...20170615_FeeCalculation_structclass) https://github.com/bitcoin/bitcoin/pull/10598
< fanquake>
wumpus heh have to follow up with a quick 0.14.3 then.
< wumpus>
yes - 0.14.2 just has to get out because of the upnp vuln.
< jonasschnelli>
ryanofsky: Yes. Saw it... as soon as I have a bit more time (currently occupied with DigitalBitbox work) i'll give you response...
< jonasschnelli>
The GUI responsiveness is ugly right now...
< jonasschnelli>
I wonder moving everything to ZMQ/RPC would be a viable approach (fully detach the GUI)
< jonasschnelli>
Ideally a clone of qt/ (to /qtdetatched)
< jonasschnelli>
Then it would allow to remove some functions which are not possible via ZMQ/RPC
< ryanofsky>
is that different than what I am doing with 10244?
< ryanofsky>
10244 removes direct calls from qt -> node wallet and replaces then with calls through interfaces which could be implemented locally or through any rpc mechanism
< wumpus>
ryanofsky: but in itself that won't improve responsiveness, or does it also make callbacks asynchronous?
< wumpus>
I still have to look at it in detail, I think the idea makes sense though
< ryanofsky>
it doesn't affect responsiveness, though i do think the improved code organization would make improvements easier
< ryanofsky>
10244 by itself doesn't change any behavior, all it does is replace direct calls with calls through interfaces
< wumpus>
right
< wumpus>
which makes sense
< wumpus>
and when there's interfaces, we can add signals to them, to subscribe to to make the UI updates asynchronous
< wumpus>
why rename CFeeBumper to FeeBumper though?
< wumpus>
it would be better if this was separated into GUI changes, and core changes, I think in current state this is way too large
< ryanofsky>
or even if we don't do that, it makes it easier to identify where blocking calls are happening, because they will all look like m_ipc_node->something() or m_ipc_wallet->something()
< ryanofsky>
the feebumper commit is the one commit that actually makes substantial core changes, i can pull that into a separate pr
< wumpus>
but why rename it?
< ryanofsky>
the other commits are almost entirely changes in src/qt and additions to src/ipc
< ryanofsky>
i renamed it because the commit had to update every single reference to it anyway, so i figured it'd be good to follow the current naming convention
< wumpus>
it seems a bit too much to try to do all that in one PR, in my opinion, but don't know what others think
< ryanofsky>
anyway happy to pull that commit into a separate pr
< ryanofsky>
happy to break it down
< ryanofsky>
but just know that one commit is an anomoly
< ryanofsky>
the other commits are uniform mechanical changes
< wumpus>
I don't personally particularly care if there's a large GUI change, but core changes need to be reviewable easily, so combining functionality changes with a rename will make it harder
< wumpus>
ok, that's good
< ryanofsky>
ok, will pull that commit out
< ryanofsky>
there is another smaller change which affects locking in wallet.cpp. will make a separate pr for that too. after this 10244 should only touch src/qt/ and src/ipc/ files, no core files
< ryanofsky>
jonasschnelli, still curious to hear more about your qtdetached idea, and if 10244 would help with that
< jonasschnelli>
10244 can probably help, but my first hurdle is: +2,217 −1,131
< jonasschnelli>
:)
< jonasschnelli>
ryanofsky: The general abstraction may be good... but it may be also possible though wallet-/clientmodel
< jonasschnelli>
My (probably dumb) concept would be to have RPC calls in there...
< ryanofsky>
because all the other commits (except feebumper which i will pull out) follow the same pattern as first commit
< jonasschnelli>
I guess I would first tackle the asynchrony in the GUI layer...
< jonasschnelli>
That needs to be done anyways and can have a quicker end user benefit when we would run all node communication in a designated thread
< wumpus>
my preferred priority would also be improving responsiveness first
< jonasschnelli>
Yes.
< jonasschnelli>
For that, we first need to analze what functions/locks are the worst
< wumpus>
though I think moving things to interfaces can help with that
< ryanofsky>
can you guys explain your reasoning? to me it seems like this is basically unrelated, but could only make things easier not harder
< wumpus>
as then it's clearer what the interface to the core thread should be
< jonasschnelli>
Turn those expensive calls into async and use a general NODE<->GUI thread?
< ryanofsky>
is there some way that my changes could make async improvments harder that i'm not seeing?
< jonasschnelli>
No... i don't say you change is not good
< wumpus>
not AFAIK
< jonasschnelli>
I'm just arguing about priorities
< jonasschnelli>
To get a +2,217 −1,131 change in, my experience tells me, ~0.5-1y
< jonasschnelli>
but ryanofsky change makes sense.. the additional layer *IPC* <-> *wallet/client-model* <-> GUI is probably okay...
< ryanofsky>
well hopefully it will take less than 0.5y to decide whether it's okay or not :)
< jonasschnelli>
hehe... I think the concept is good.
< ryanofsky>
feedback so far is helpful though, definitely makes sense to keep all core changes out of the pr and make it qt-only
< jonasschnelli>
Yes. Ideally... this would reduce some risks
< wumpus>
agree with doing it after the 0.15 split-off
< fanquake>
to early for a 0.16 tag heh
< bitcoin-git>
[bitcoin] practicalswift closed pull request #10597: scripted-diff: Make use of the improved handling of two consecutive right angle brackets in C++11 (master...right-angle-brackets) https://github.com/bitcoin/bitcoin/pull/10597
< jonasschnelli>
ryanofsky wumpus: is it only me or is the responsiveness in the current GUI worse then 0.13?
< jonasschnelli>
ryanofsky: I guess some minor fixes could speed up the GUI and there would still be time for 0.15
< fanquake>
jonasschnelli in 0.14.2 or master?
< jonasschnelli>
fanquake: I always run master and I often have some freezes...
< jonasschnelli>
I though my balance fix would reduce some... but sadly no
< ryanofsky>
i can't say because i didn't really use the gui in 0.13, and even now tend to only use gui with small wallets, few transactions
< jonasschnelli>
Would a LogPrint (with microseconds) in our LOCK macro make sense to analyse the lock behavior better?
< ryanofsky>
that seems like would probably tell you where freezes are happening, maybe there is also a way to hook into qt to warn about event handlers that take a long time to run
< fanquake>
jonasschnelli during any particular actions?
< jonasschnelli>
fanquake: the freezes or the log print?
< jonasschnelli>
ryanofsky: Yes...
< fanquake>
the freezes
< jonasschnelli>
fanquake: Yes. Calling generate 1 followed by a sendtoaddress feels blockish
< wumpus>
I think there's too much overhead from logging to make that useful, also logging itself also locks
< wumpus>
so you'd have to exclude that; but you could try
< jonasschnelli>
I guess i'd use printf for a short hackish solution
< wumpus>
in any case, I'm not sure how much you'd learn, all LOCKs in GUI code and calls to core functions that take locks on cs_main are a problem. THough possibly you can find the ones that occur most frequently if you also log where it's taken.
< bitcoin-git>
[bitcoin] practicalswift opened pull request #10602: Make clang-format use C++11 features (e.g. A<A<int>> instead of A<A<int> >) (master...clang-format-cpp11) https://github.com/bitcoin/bitcoin/pull/10602
< jnewbery>
Is anyone using the 'comparison' part of the comparison test framework? Any objections to me retiring it? #10603
< wumpus>
jnewbery: I'm fairly sure people are using it
< wumpus>
@(sdaftuar morcos BlueMatt)
< sdaftuar>
wumpus: i've talked to jnewbery about it offline a bit; i mostly think we don't really use it
< wumpus>
okay
< sdaftuar>
the intention was to be able to compare many versions of the software
< wumpus>
but that isn't that useful in practice?
< sdaftuar>
but i don't think anyone (outside my proof of concept, way back when) actually tried to do that?
< wumpus>
would be a better question to ask then "should someone be doing those tests?"
< sdaftuar>
yeah i think that's the good question
< jnewbery>
The way those test cases are actually used in practice is that test_runner.py runs them with a single node. In theory they could be run with multiple nodes and the states compared, but as far as I'm aware no-one does that
< wumpus>
I mean we could always add tests, maybe even in a travis crontab
< sdaftuar>
in practice, it seems like we've never really written tests where the outcome wasn't fixed and known, and therefore hardcoded in the test itself
< sdaftuar>
i guess it's possibel we could introduce a change to bitcoind and the corresponding test and accidentally introduce a comparison failure
< sdaftuar>
p2p-fullblocktest doesn't change very often though
< sdaftuar>
the risk seems like it'd be in consensus changes not yet deployed, eg something like segwit
< sdaftuar>
but those tests haven't changed much either
< jnewbery>
I think the risk seems low. The individual tests are written quite prescriptively, so in order to break a 'comparison' without breaking running the test with an individual node, someone would probably need to update the individual test case to change the expectation
< jnewbery>
Anyway, I'd be interested to hear whether anyone out there is using them or has any input. Issue 10603
< jnewbery>
The tests that use the comparison test framework are:
< gmaxwell>
sdaftuar: we used it extensively in the past.
< gmaxwell>
rather the old bitcoinj tool. (to be clear)
< sdaftuar>
gmaxwell: right, we're still going to have p2p-fullblocktest
< sdaftuar>
which is approx. the same as the old bitcoinj tool
< sdaftuar>
the question is just, do we try to support/improve the ComparisonTestFramework thing as the implementation
< sdaftuar>
which i think has mostly been a not very helpful infrastructure... the tests in that framework are hard to read
< gmaxwell>
OK.
< sdaftuar>
and working around p2p changes that the ComparisonTestFramework isn't designed for is pretty hacky
< bitcoin-git>
[bitcoin] jnewbery opened pull request #10604: Expose multiwallet in getwalletinfo and add multiwallet test (master...multiwallet_test) https://github.com/bitcoin/bitcoin/pull/10604
< gmaxwell>
sorry, I tuned in to the end of the conversation. I mostly started commenting because I strongly disagree with the claim that that the existing test cases were at all close enough to comprehensive to guarentee consensus consistency with another implementation. We don't even come close to 100% branch coverage in script.
< sdaftuar>
oh, yeah definitely agree with that
< sdaftuar>
i think the question is, should we be working towards comparison style tests, along the lines of what the comparisontestframework was intended for?
< kanzure>
branch coverage would be a good set of tests..
< kanzure>
er, script coverage. what is missing?
< gmaxwell>
sdaftuar: I think they're secondary to other kinds of tests. Comparison is primarily useful if we have good random behavior generation and can verify consistency.
< sdaftuar>
that makes sense to me... my thought was that it would be more helpful to rewrite our comparison tests (which are currently just evaluating a single node anyway) in an imperative style, to make them easier to maintain and debug
< sdaftuar>
and in retrospect, i think if we did want a comparison test framework, it probably makes more sense to scrap trying to do it over p2p, and just test consensus with submitblock or something. less prone to maintenance headache
< gmaxwell>
er. I don't agree (sadly)--
< gmaxwell>
for two reasons: p2p means its more duarable across versions (e.g. you can test an alternative implementation), and p2p means its testing the interface that actually matters; it would catch thing like failures in the block fetching state machine.
< gmaxwell>
Comparing other implementations can increase the sensitivity of the tests even if we'd never expect someone to use the other implementation.
< sdaftuar>
that's true in theory, but i think that has mostly failed in practice
< sdaftuar>
at least, we have never implemented the full range of p2p behaviors in a single piece of python test harness code
< wumpus>
what I'm not sure about is how the comparision test is better than just running the tests against the other implementation
< sdaftuar>
so for instance, we end up implementing kludges and workarounds in the python test code in order to eg ensure block delivery to a particular implementation. see for instance how we handle announcing and relaying a block, after we turned off direct fetch on inv's in bitcoind
< sipa>
wumpus: i guess the advantage is that you can randomly generate scenarios and compare them, rather than needing a test writer who knows what the result is supposed to be
< gmaxwell>
wumpus: see my comment about randomly generated behavior. If you have a random sequence of operations you can test certian invariants, but the random generator doesn't know the exact result-- but it can let us know when two different implementations behaved differently.
< sipa>
but that advantage does go away if you're just running a static scenario
< jnewbery>
gmaxwell: I think really for stability across versions, you want to have a known good node that you use RPCs on to create blocks and transactions, then connect that node to nodes-under-test of different versions. Otherwise you need to update the tests whenever any p2p logic changes (eg headers first, etc)
< gmaxwell>
It's an orthorgonal and very powerful method of testing.
< sdaftuar>
it seems to me that the more durable thing is to have specific p2p tests that exercise the code paths we want to test
< wumpus>
right, if you can somehow generate random scenarios, it'd be useful
< gmaxwell>
And yes, for static tests is not useful IMO.
< wumpus>
yes, then it's like fuzzing two implementations at the same time and comparing
< gmaxwell>
Rather if we get interesting patterns out of a comparison, we could seralize those into a static test (and add the exact comparison points)
< wumpus>
ProfMac: where doesn't matter, as long as you consistently use same the directory, though cloning bitcoin under the vm-builder directory likely is not what you want
< gmaxwell>
This is, e.g. how we tested the DER parser in libsecp256k1-- a harness with three implementations.
< jnewbery>
ok, so how about I change those static tests to use the standard test framework? I won't delete the comparison test framework code so if someone wants to use it for randomized testing, they can still do that
< ProfMac>
Thanks wumpus. I am doing a 2nd install from scratch, yesterday I could not get it to work.
< sipa>
jnewbery: if all they are doing is testing a scenario where it's obvious from the test code what the expected behaviour is, sure
< gmaxwell>
Would that mean moving the test stimulus for those things from p2p to rpc?
< sdaftuar>
no, it shouldn't
< gmaxwell>
okay!
< wumpus>
that would mean that the comparison test code doesn't get tested itself anymore, and thus will code-rot
< sdaftuar>
wumpus: i fear it already has rotted :(
< ProfMac>
So I'm going to continue with the 3 git clone commands in "~/opt/gitian", anyone see any red flags or offer a better naming scheme?
< wumpus>
ok...
< sdaftuar>
i mean that in the sense, we already have hacky workarounds in eg p2p-fullblocktest to deal with shortcomings in the framework
< gmaxwell>
meeting in 10 minutes.
< jonasschnelli>
hi
< wumpus>
#startmeeting
< lightningbot>
Meeting started Thu Jun 15 19:00:07 2017 UTC. The chair is wumpus. Information about MeetBot at http://wiki.debian.org/MeetBot.
< luke-jr>
basic multiwallet was merged. I hope to have the next step (RPC support) later today. ACK to add to priority then?
< jonasschnelli>
RPC support would be great
< sipa>
agree
< jonasschnelli>
You mean addressing wallet via RPC endpoint?
< achow101>
luke-jr: yes please
< luke-jr>
jonasschnelli: I mean each username has a different single wallet
< jonasschnelli>
luke-jr: hmm...
< wumpus>
sipa: ok, removing it from high priority then
< jonasschnelli>
I'd prefere endpoints
< jonasschnelli>
AUTH for wallet switching seems hackish
< luke-jr>
jonasschnelli: endpoints can be done later; I'm just trying to get the simplest stuff done first
< jonasschnelli>
endpoint is 10lines of code
< wumpus>
I'd also prefer endpoints, makes it easier to move wallets to external processes etc just by changing the url
< jonasschnelli>
I can post it to you later
< luke-jr>
jonasschnelli: not securely ;p
< luke-jr>
jonasschnelli: I don't want JoinMarket to have access to my main wallet
< jonasschnelli>
there is no security in our RPC implementation :/
< sipa>
yay, wallet ACLs
< * sipa>
hides
< wumpus>
please don't use auth, it's not supposed to be a multi-user authentication mult-wallet, that just adds another nightmare difficult to support like accounts
< achow101>
you could do some combination of both?
< luke-jr>
wumpus: different username is also a simple change to the URI
< wumpus>
we already support different usernames/passwords
< jonasschnelli>
use a passphase as wallet name
< wumpus>
it's an authentication feature, should not affect the wallet
< jonasschnelli>
same security as basic auth?
< sipa>
i think the first step should be either endpoint or a generic optional named parameter to select the wallet
< luke-jr>
wumpus: I see no distinction
< wumpus>
sipa: yes
< gmaxwell>
endpoints won't be ten lines of code. After all, we'll need to add support for them to bitcoin-cli, the test framework, etc.
< achow101>
sipa: agreed
< sipa>
the choice of which user can access which wallets is orthogonal, i think
< wumpus>
gmaxwell: which is easy, the underlying stuff (rpcproxy, libevent) obviously supports it
< sipa>
but i would prefer not to tie users to wallets at the auth level
< jonasschnelli>
gmaxwell: yes Indeed
< jonasschnelli>
first stage, each user should be able to access each wallet....
< gmaxwell>
obviously we do want to have username/wallet binding, right? This lets you be more confident e.g. that your joinmarket install isn't going to screw up your ordinary wallet, for example.
< wumpus>
sipa: me neither, it just seems a level violation, and causes wrong expectations that giving access to RPC to one wallet is secure in any way
< gmaxwell>
(eventually)
< luke-jr>
jonasschnelli: no -.-
< wumpus>
gmaxwell: I really think that's going too far
< gmaxwell>
then why are we bothering?
< wumpus>
securing RPC for multiple users is absolutely a nightmare
< jonasschnelli>
luke-jr: the first logical extendable step would be that, no? Adding wallet selecting via AUTH is something you need to throw away later
< luke-jr>
well, if I can't isolate JoinMarket this way, I have no interest in doing it.. so I can just move on to GUI and leave RPC support in Knots only
< sipa>
wumpus: i think it's inevitable that we'll need that
< wumpus>
anyhow a security layer could always be added could be later if endpoint-based multiwallet is in place
< wumpus>
sipa: I think it's a mistake
< wumpus>
sipa: just like accounts was
< wumpus>
it's something that bitcoind shouldn't handle
< gmaxwell>
I think what luke would like to accomplish is making multiwallet immediately useful for the application of combining multiple applicatoins onto one bitcoind; rather than having to run seperate bitcoinds for each thing that needs a wallet that you're running.
< luke-jr>
jonasschnelli: no?
< sipa>
gmaxwell: i think that's an interesting use case; i don't think it should be the first step
< wumpus>
that's just inviting bugs, there's no way we can make that secure, the RPC is not a secure endpoint and is regarded as compeltely trusted
< wumpus>
it would escalate a bug in e.g. a single RPC command to a security issue, right now RPC access = fully trusted
< jonasschnelli>
luke-jr: I don't know the JoinMarket use case very well.. but if you give it access to your node, it could shutdown, add peers, etc. (in case you don't trust that software)
< gmaxwell>
This is also important to us at blockstream and we will end up maintaining a fork of Bitcoin with it. (though luke wasn't doing this work at our request).
< luke-jr>
jonasschnelli: even if we add endpoint multiwallet and ACLs later, we still want a way to select a default wallet for each user
< luke-jr>
wumpus: then why do we have auth at all?
< jonasschnelli>
I really think we should keep hands away from multi-user/multi-wallet setup
< wumpus>
luke-jr: to gain access
< wumpus>
jonasschnelli: me too... seems something that needs to be a level on top, not handled by bitcoind itself
< jonasschnelli>
For now we should focus on single-user/multi-wallet (1:n)
< jonasschnelli>
n:n smells like a account-like-problem-re-incarnation
< wumpus>
anyhow if we have endpoint multi-wallet access, it'spossible to slap on a wallet/user auth mapping later
< luke-jr>
or vice-versa..
< wumpus>
that's "just" a matter of access control
< jonasschnelli>
yes. n:n may make sense.. but endpoint first seems much more logical
< wumpus>
yes
< gmaxwell>
jonasschnelli: I do not follow your comment with account like problems. The problem with accounts is that they weren't wallets but users expected them to be and treated them like ones.
< jonasschnelli>
gmaxwell: Yes. Not directly related.
< sipa>
i don't think access control is necessarily that complicated; have a global permission and wallet specific permission; configure which users have which
< wumpus>
I just think that making bitcoind multi-user is a grave mistake
< wumpus>
but I"ll shut up about it...
< jonasschnelli>
I think the complexity is huge,.. leads to permission groups, etc.
< wumpus>
yes, exactly, some people wnat everything in bitcoind
< gmaxwell>
jonasschnelli: what? no it doesn't.
< sipa>
well it seems that multiple people want multiwallet for multiple reasons
< sipa>
i don't think that's a problem
< sipa>
and should not be a blocker for the basic functionality
< wumpus>
this is one the reason why the wallet should have been split off to a separate process / library I guess... now it all needs to be compounded
< wumpus>
making bitcoind some kind of systemd
< jonasschnelli>
if we start to use n:n, enterprises will probably use it for multi-user wallet backends...
< luke-jr>
wumpus: user:wallet makes a split off later simpler
< jonasschnelli>
and removing – if it gets to complicated – is hard or even impossible (like the accounting)
< wumpus>
jonasschnelli: yes exactly... and what if there's a bug in that
< luke-jr>
endpoints makes splitting off later complex
< wumpus>
it moves all the (perceived) responsiblity for managing multi-user setups secure to us
< jonasschnelli>
luke-jr: endpoint would even work if each wallet runs in its own process
< gmaxwell>
How do we split wallets if we are using endpoints?
< jonasschnelli>
(though auth probably also)
< luke-jr>
jonasschnelli: huh? not really..?
< jtimon>
but multi-wallet doesn't imply multi-user, does it?
< wumpus>
gmaxwell: what do you mean with "split wallets"?
< gmaxwell>
split wallets int oseperate processes
< wumpus>
gmaxwell: different wallets have different URLs then
< wumpus>
gmaxwell: so it's just another change: change the port...
< luke-jr>
wumpus: ALL of these options are simple URI changes..
< achow101>
how would different endpoints work with bitcoin-cli or the debug console?
< luke-jr>
although some tools don't allow changing the URI right now
< luke-jr>
one in the main window, and one in the debug window
< gmaxwell>
sounds more or less okay.
< gmaxwell>
luke-jr: so why not implement endpoints first? surely even if your own use needs account you can carry a 5 line patch to allow accounts to select the default wallet.
< luke-jr>
wumpus: even if a single user can access multiple wallets, we still want a way to choose the default
< gmaxwell>
see above
< luke-jr>
gmaxwell: it's more code, and not done yet
< wumpus>
well the default wallet could depend on the user, I don't really care
< luke-jr>
I can implement it, but IMO it will delay things to make it the next step
< wumpus>
though I'd prefer to get rid of 'default wallet', in time
< jonasschnelli>
maybe the GUI should have a node window (network, peers) and a wallet-window per wallet...
< sipa>
jonasschnelli: ugh
< luke-jr>
the user=>wallet stuff is literally done and well-tested (in Knots), just needs to be rebased
< gmaxwell>
jonasschnelli: that doesn't sound like a good UI. :P
< sipa>
jonasschnelli: /me remembers browsers before tabs
< gmaxwell>
at least not mandatory.
< gmaxwell>
what sipa says. :P
< jonasschnelli>
yeah... I like windows.. but I'm pretty alone nowadays with that
< jonasschnelli>
Yeah. Tabs make more sense I guess.
< sipa>
anyway, separate discussion
< * jonasschnelli>
think sipa certainly browses with lynx
< sipa>
i would really prefer endpoints or optional named argument to select a wallet, and deal with the authentication question later
< jonasschnelli>
sipa: +1
< gmaxwell>
luke-jr: in any case, seems to me the path forward is to do the endpoints thing, and having auth pick default is a simple change which is either sufficiently non-objectionable or at least a trivial patch to carry.
< achow101>
sipa: ack
< wumpus>
sipa: same for me
< luke-jr>
would anyone NACK if I go forward with user->wallet mappings since they're basically ready, and then do endpoints based on that?
< achow101>
probably
< gmaxwell>
luke-jr: does it also support one user with many wallets?
< wumpus>
well as we determined above, a user may want to have access to multiple wallets, so a single user->wallet mapping just doesn't cut it, even if you want to add access control
< gmaxwell>
what wumpus says.
< luke-jr>
gmaxwell: the current code does not, but there's no reason the endpoints couldn't add that
< wumpus>
I really think we should just start with endpoints as sipa says
< jnewbery>
Isn't rpcuser deprecated anyway?
< jonasschnelli>
Yes. Let's start with endpoint.. I'll can write it next week because I already did once...
< wumpus>
I'm not going to NACK anything that makes progress though
< luke-jr>
jnewbery: it's rpcauth
< sipa>
jnewbery: rpcauth isn't
< jonasschnelli>
*I can
< gmaxwell>
jnewbery: rpcuser is, but this is rpcauth (rpcuser doesn't even have multiple users)
< jtimon>
jnewbery: is rpcuser deprecated? since when?
< gmaxwell>
jtimon: a year?
< achow101>
jtimon: it's deprecated since a long time ago
< gmaxwell>
it prints out a notice!
< wumpus>
rpcuser is deprecated, people are encouraged to use either rpcauth or cookie auth
< wumpus>
we won't remove it just yet ofcourse
< jtimon>
oh, deprecated as in "we want to remove this", but it actually still works, no?
< paveljanik>
26 minutes...
< luke-jr>
right
< achow101>
yes
< wumpus>
that is what deprecated means, yes
< jtimon>
yeah, sorry
< instagibbs>
paveljanik, 34 to go :P
< wumpus>
paveljanik: what's so special about 26?
< sipa>
jonasschnelli: any progress on GUI for database upgrade?
< wumpus>
#topic GUI for database upgrade?
< gmaxwell>
And we've wasted a perfectly good half hour. :P luke should put up patches and we can yell at him on github, but I think I would really prefer if the first cut does multiple wallets for a user... (if nothing else, that is the easiest thing to test)
< jonasschnelli>
sipa: I sadly had only little time last and this week
< luke-jr>
wumpus: 21 is half of 42
< jonasschnelli>
sipa: I looked into it and wanted to ask you how I get the max size of a db cursor (to calc progress)
< sipa>
jonasschnelli: it's not hard to estimate as txids are randomly distributed
< gmaxwell>
so you just look at the txid...
< sipa>
i can add code for that
< gmaxwell>
they're done in order.
< gmaxwell>
if it's at 0x01... then it's done 1/256 of it.
< sipa>
txid -> arith_uint256 -> * 100/2^256
< jonasschnelli>
Okay. The rest is simple (debug.log non newline [10%] progress / GUI splash screen progress with abort)
< wumpus>
BTW: jnewbery rebased the label API pull (#7729), a lot of thanks for that
< wumpus>
concept ack (haven't gotten around to reviewing anything)
< gmaxwell>
I think those changes all make sense. Someone commented about breaking compatibility, but its for a new major version and it will be easy for callers to update their behavior.
< morcos>
i'm here now.. haven't caught up on backlog
< sipa>
it breaks compatibility with a never documented or advertized feature :)
< wumpus>
well we could allow both, for one major version
< gmaxwell>
Though we might want to rename the old calls at the same time. (a suggestion for discussion)
< wumpus>
ok, we're talking about different things
< instagibbs>
aside from mixing wallet/nonwallet, what's the issue with validateaddress?
< gmaxwell>
sipa: it was certantly advertised and known.
< wumpus>
I mean validateaddress/getaddressinfo
< instagibbs>
(or is that the issue)
< achow101>
instagibbs: that's the issue
< sipa>
wumpus: ah, i'm talking about signrawtransaction
< gmaxwell>
wumpus: he's talking about the signraw split to create the combine call.
< wumpus>
instagibbs: that is the issue
< instagibbs>
ok, are we killing off getinfo then too:
< instagibbs>
:)
< achow101>
hopefully :D
< wumpus>
instagibbs: yes, but that's not the topic now
< gmaxwell>
I would miss getinfo. all the other commands take more typing. :P
< wumpus>
we're already confusing two things, let's add more!
< gmaxwell>
wumpus: okay!
< instagibbs>
eh, we're talking about blowing away validateaddress as is, sorry
< wumpus>
gmaxwell: hey I have a pull that implements it client side
< gmaxwell>
wumpus: lol. can you name the rpc call "gi" :P even less typing.
< jonasschnelli>
IMO having a non wallet sign rawtx where priv keys are passed aroung in a shell over a possible TCP channel is not ideal.. but we already have it... I though instead of splitting it off, move it to the tool
< jonasschnelli>
But I see the point with getting the UTXOs
< sipa>
let's rename all RPCs to get*info... for example s/sendtoaddress/getnewpaymenttxid/
< gmaxwell>
jonasschnelli: we need the UTXOs or its all awfulsauce.
< jonasschnelli>
gmaxwell: I though the node RPC can spitout what you need to pass it into bitcoin-tx or so...
< jonasschnelli>
but I know... very inconvinient
< luke-jr>
getthistransactionbroadcast
< luke-jr>
:p
< sipa>
jonasschnelli: sure it can; it's just more convenient to not need that
< wumpus>
eventually closed it because the only person responding was luke-jr and he kept arguing against it
< gmaxwell>
jonasschnelli: and that adds steps to the process.. which is already long enough that it's prone to error.
< sipa>
jonasschnelli: it's called listunspent
< jonasschnelli>
It's just another source how people can shoot themselfs with exposing priv keys
< gmaxwell>
luke-jr: why do you hate freedom?
< luke-jr>
gmaxwell: !⁈
< gmaxwell>
haha
< sipa>
jonasschnelli: but the functionality already exists, and i very much like removing it from the wallet (so people at least won't accidentally mix up privkey based operations with wallet stuff)
< instagibbs>
sipa, ok I see the motivation there
< gmaxwell>
anyways, I can just say that I have tried to stop using getinfo and failed. Mostly because typing getnetworkinfo getblockchainfo getfooooooooooinfo and then wading through a bunch of things when I want to see: How many connections, which block am I at, and what wallet am I running (which I can tell via the balance). :P just personal feedback.
< luke-jr>
gmaxwell: I didn't even NACK it :o
< jonasschnelli>
Yes... I guess that makes sense. I kinda hoped once we touch that we could move it away from the node into a sep. process
< luke-jr>
gmaxwell: use the GUI for that! :p
< sipa>
jonasschnelli: i have a vague proposal for that too, but it's more complicated
< gmaxwell>
luke-jr: again, why do you hate freedom? :P
< sipa>
jonasschnelli: and involves a new format for partially signed transactions...
< wumpus>
gmaxwell: anyhow we can easily reopen and rebase that PR, bitcoin-cli hardly changed since then
< sipa>
wumpus: ack
< jonasschnelli>
sipa: that contains everything you need to sign?
< luke-jr>
gmaxwell: make a shell alias to all the calls ;)
< gmaxwell>
it's so much easier to have a signing blob thing post segwit. :(
< gmaxwell>
luke-jr: I don't like customizing my expirence of bitcoin too much because then I'll just patch around everything that stinks.
< * sipa>
revives segnet
< jonasschnelli>
sipa: That's also something we could re-use for the detatched signing standard (a.k.a hardware wallet standard)
< luke-jr>
we could just leave getinfo how it is ;)
< gmaxwell>
in any case, we're offtopic. I think that achow's PRs are all nice incremental improvements and we should take them (after review)
< sipa>
jonasschnelli: pre-segwit however, it also needs to contain the full spent transactions :(
< luke-jr>
bitcoin-cli -getinfo only handles 1 sortof-use-case, and leaves the other 2 supported use cases unaddressed
< sipa>
gmaxwell: agree
< wumpus>
after promising to deprecate it for years... yeah, of course....
< sipa>
(disclaimer: achow101'w my intern this summer, i asked him to work on those)
< jonasschnelli>
sipa: [full spent transactions], I guess that's okay.
< sipa>
jonasschnelli: bitcoind unfortunately can't do that generically (you need the wallet for that)
< wumpus>
getinfo is going away, there's no going back now
< luke-jr>
let's add a getallinfo then
< luke-jr>
/s
< wumpus>
I've exactly documented what information you can find on what get*info command
< gmaxwell>
I am fine with it going away, but I don't believe we replaced it as well as we thought we did.
< wumpus>
and the client-side getinfo is ther for user friendlyness, if people want it
< sipa>
i have no problem with removing it, with or without 8843
< luke-jr>
wumpus: it doesn't work in the debug window
< sipa>
i'm sure i'll curse a bit that getinfo isn't around anymore, and then change my habits
< gmaxwell>
sipa: I tried blocking it, you won't. the replacements right now are not usable.
< luke-jr>
:/
< gmaxwell>
But thats okay, wumpus suggestion would be fine, though luke has a point about the debug console.
< wumpus>
luke-jr: isn't all the information in the debug window *without* typing anything?
< achow101>
removing getinfo will mess with a ton of things that use getinfo for basic rpc connection checking too...
< wumpus>
achow101: and you're starting to bring that up *now*?
< achow101>
but I think it should be removed anyways
< sipa>
let's merge the getuptime rpc thing
< gmaxwell>
wumpus: if it isn't we should make it. problem solved.
< bitcoin-git>
[bitcoin] ryanofsky opened pull request #10605: Add AssertLockHeld assertions in CWallet::ListCoins (master...pr/listlock) https://github.com/bitcoin/bitcoin/pull/10605
< instagibbs>
achow101, move to dumpprivkey obv
< wumpus>
gmaxwell: the first tab of the debug window pretty much shows everything, and indeed, if it isn't it could be added
< sipa>
oh, another topic: non-hardened key derivation
< achow101>
wumpus: well most of those things are old website scripts that were written once and never touched again by the authors
< instagibbs>
sipa, ACK
< gmaxwell>
luke-jr: I think ^ is how we should handle the gui. (also important to make it copy/pasteable if it isn't.)
< wumpus>
I'm really disappointed that years after deciding to deprecate getinfo we're still having this discussion
< luke-jr>
wumpus: lol maybe :D
< wumpus>
anyhow next topic
< gmaxwell>
wumpus: sometimes you have to try things out to know their effects completely!
< wumpus>
#topic non-hardened key derivation
< sipa>
so
< sipa>
non-hardened key derivation has many use cases in addition to hardened
< jonasschnelli>
I guess NicolasDorier made good work there
< wumpus>
achow101: we should carefully note it in the release notes of course
< sipa>
however, they also have a gaping wide security hole when child private keys are exposed
< wumpus>
achow101: we could even make getinfo fail with a custom message
< instagibbs>
that plus dumpwallet will give you sads
< sipa>
thus, suggestion: allow a new wallet to be created with either harderned or unhardened keys
< sipa>
when you choose unhardened, dumpprivkey is disabled
< instagibbs>
xpub is only accessible through dumpwallet right now AFAIK
< achow101>
wumpus: returning null would probably not mess with anything
< sipa>
(but dumpmasterkey or whatever is still available)
< achow101>
sipa: there is no dumpmasterkey
< jonasschnelli>
instagibbs: dumpprivkey must be disabled anyways
< luke-jr>
sipa: I'd want to be able to mix them..
< jonasschnelli>
achow101: dumpmasterkey must be added
< instagibbs>
luke-jr, .... why
< gmaxwell>
this is a lot more interesting with multiwallet support in place, since the cases where you want that are mostly secondary wallets (like incoming payments with keys generated by your webserver)
< wumpus>
dumpmasterkey, isn't there a PR for that?
< jonasschnelli>
no
< luke-jr>
instagibbs: to generate reusable payment tokens
< sipa>
luke-jr: i guess that's fine; just disable dumpwallet, and dumpprivkey selectively for keys derived in a non-hardened fashion
< gmaxwell>
sipa said or whatever for a reason. :P
< achow101>
jonasschnelli: I'm stil lwaiting #9504
< gmaxwell>
wumpus: well, technically it doesn't prove that...
< jonasschnelli>
achow101: how can I not be aware of that PR... sorry for the missinfo
< wumpus>
gmaxwell: true
< sipa>
getmissinfo
< jonasschnelli>
:/
< jonasschnelli>
(I even commited on the PR ^^)
< jonasschnelli>
commented
< wumpus>
getmisinfo would really fit with the spirit of the times
< gmaxwell>
Anyways, I think that sounds okay. These applications will likely need a couple of extra RPCs too. no need to design here however. (e.g. it will need to export the extended public key.)
< sipa>
not much more to say about the topic - just pointing out that if we disable dumping child private keys, my concern with non-hardered derivation largely goes away
< wumpus>
anyhow, no problem with non-hardened key support
< jonasschnelli>
I guess with allowing xpub derivation, flexible keypath would be welcome..
< jonasschnelli>
People want to use BIP44
< wumpus>
as an option, not as default
< gmaxwell>
w/ disabling and it not being a default thing. sounds great to me.
< achow101>
sgtm
< jonasschnelli>
Yes. Would be a great change...
< gmaxwell>
also so long as we do the extra rpcs to make it actually useful (like extract the extended public keys, and whatever else is needed to handle an external address generator.)
< jonasschnelli>
I guess keypool handling would be much simpler with xpub derivation
< gmaxwell>
I doubt it?
< instagibbs>
I think it's the same-ish
< jonasschnelli>
Why would you need a keypool with xpub derivation?
< gmaxwell>
for scanning.
< jonasschnelli>
you derive when you need
< instagibbs>
you have master seed already, you can already do that
< jonasschnelli>
you always derive the lookup window in mem
< gmaxwell>
you need to scan forward to know when you're paid.
< jonasschnelli>
+1000 keys in mem should be ackish
< jonasschnelli>
(pubkeys)
< gmaxwell>
yes, the keypool doesn't technically need to be saved on disk, though it may be faster to do so then to generate thousands of addresses at every wallet load.
< gmaxwell>
but otherwise I think its the same.
< jonasschnelli>
I'd say loading the keypool at wallet load takes almost the same (or longer)
< jtimon>
sipa: why would people want to dump child private keys?
< gmaxwell>
okay, this would make the change much more intrusive then I think.
< instagibbs>
jtimon, you want to stop them from doing so
< gmaxwell>
jtimon: same reason some people eat paste. (they don't know better)
< jonasschnelli>
heh
< sipa>
jonasschnelli: the reason creating new keys is slow, is because we flush them all individually to disk
< wumpus>
yes...
< jtimon>
gmaxwell: I see
< jonasschnelli>
Yes. In-mem non-bdb "keypool" would be much faster
< gmaxwell>
that flushing is not required anymore with hdwallets regardless, I believe.
< sipa>
gmaxwell: exactly
< jonasschnelli>
With xpub derivation, would there be really a need to write the pool to disk? I doubt it
< gmaxwell>
(we will still need a flush to know how many we've given out...)
< wumpus>
gmaxwell: good point!
< jonasschnelli>
All you need is the seed
< gmaxwell>
jonasschnelli: no, not a need, though why result in more distinct codepaths? I would expect it to also make loading faster.
< achow101>
you just need to write the path of the most recent key
< jonasschnelli>
gmaxwell: yes. thats a good point
< jonasschnelli>
achow101: Yes.
< gmaxwell>
achow101: yes, every new address still needs to do a wallet flush. But on bulk key creation we do not need a flush for each operation.
< gmaxwell>
e.g. starting up for the first time with a keypoool of 10000.
< wumpus>
two minutes to go
< gmaxwell>
jonasschnelli: did you ever get around to implementing the change where any key that is noticed used on the blockchain marks all the earlier keys in its chain as used?
< wumpus>
gmaxwell: just don't want to talk with luke-jr about it :p
< gmaxwell>
I think the main challenges for it in an interactive setting is that the replacements spam the screen with information which is not what I'm interested in, and it takes about three commands to check on them. Thats not reason to not get rid of getinfo at all, and I support getting rid of it, but think we need something like your PR. I'll give it a look.
< gmaxwell>
haha
< gmaxwell>
Well, he does hate freedom, after all.
< sipa>
gmaxwell: perhaps you should write a getmyinfo.sh with just queries for the things you're typically interested in :)
< wumpus>
I mean the reason for deprecating it is that we didn't want to add any more info to it, also it combines wallet and non-wallet information which doesn't work with multiwallet
< wumpus>
sipa: I have tons of those scripts
< wumpus>
e.g. to show # connections per interface in/out
< wumpus>
getinfo's output isn't *that* useful
< luke-jr>
maybe -getinfo should take a -getinfodata which can go in the config file, to tell it what data to fetch from what RPCs ;)
< wumpus>
luke-jr: well in that case I'd prefer just a python script
< ProfMac>
What git version introduced the file bitcoin/contrib/gitian-build.sh ?
< wumpus>
oh wait I'm talking to luke-jr about getinfo
< luke-jr>
:<
< morcos>
at the risk of irritating everyone, i use getinfo all the time
< gmaxwell>
sipa: so then I'm happy while typical users suffer, I've tried to avoid doing that.
< achow101>
ProfMac: check the history in github
< ProfMac>
morcos, yep, me too.
< gmaxwell>
same reason I don't use the ncurses thing that I like.
< Chris_Stewart_5>
Why don't we return fee estimation information in sat/byte?
< ProfMac>
Thanks, achow101
< jonasschnelli>
gmaxwell: Yes. 20 is to little...
< instagibbs>
ProfMac, you can always `git log -p -M --follow -- file/path` to track those kind of questions
< jonasschnelli>
I guess I took BIP44 for a start.. but it's a single const
< instagibbs>
Chris_Stewart_5, because legacy, mostly
< wumpus>
instagibbs: hah, 99% of the time that's the right answer to any 'why' question
< Chris_Stewart_5>
I suppose that is hard to change too for fear of people paying large fees if they don't get the memo
< wumpus>
once a custom is ingrained, it usually turns out the cost and risk of change doesn't weigh up against any possible benefits
< ProfMac>
Thanks instagibbs. I'm just spinning up to speed with git. I made a deterministic vm description and a Ubuntu preseed file de novo and am now making a local git repository, then spinning up the deterministic bitcoin build. Lots of coffee and new thought patterns.
< instagibbs>
Any new interface certainly should be sat/byte
< Chris_Stewart_5>
^
< wumpus>
and then you have multiple different conventions on different RPC calls
< sipa>
nanobitcoins per Hart
< wumpus>
some of the output of getinfo is also blatantly deceptive by now, e.g. it shows one proxy, while every network can have its own proxy
< sipa>
do we still show hashrate?
< wumpus>
other things are only marginally useful; how many times have you wanted to know the the wallet version?
< wumpus>
or "testnet":false, which is also false for regtest
< wumpus>
sipa: no, that seems to be gone :
< instagibbs>
new interface meaning RPC2.0 or whatever... not new calls
< gmaxwell>
To be clear I am joking about luke hating freedom.