< bitcoin-git>
[bitcoin] TheBlueMatt opened pull request #9273: Remove unused CDiskBlockPos* argument from ProcessNewBlock (master...2016-12-remove-unused-pnb-param) https://github.com/bitcoin/bitcoin/pull/9273
< bitcoin-git>
[bitcoin] MarcoFalke opened pull request #9274: [qa] pruning: Use cached utxo set to run faster (master...Mf1612-qaPruningCacheUtxos) https://github.com/bitcoin/bitcoin/pull/9274
< bitcoin-git>
bitcoin/master 827d9a3 Wladimir J. van der Laan: qt: Replace NetworkToggleStatusBarControl with generic ClickableLabel...
< bitcoin-git>
bitcoin/master 042f9fa Wladimir J. van der Laan: qt: Show progress overlay when clicking spinner icon...
< bitcoin-git>
bitcoin/master 4d955fc Jonas Schnelli: Merge #9218: qt: Show progress overlay when clicking spinner icon...
< bitcoin-git>
[bitcoin] jonasschnelli closed pull request #9218: qt: Show progress overlay when clicking spinner icon (master...2016_11_overlay_when_clicking_sync_icon) https://github.com/bitcoin/bitcoin/pull/9218
< BlueMatt>
jtimon: re: libconsensus: have you thought more about it after the discussions in milan? I'm still not a huge fan of exposing a ton of functions and prefer a simple refactor that a) makes it so that everything ProcessNewBlock calls isnt aware of disk positioning/etc and b) move all that stuff into a class which owns mapBlockIndex, chainActive, etc
< jtimon>
I keep thinking the same thing more or less
< jtimon>
if you don't want to expose verifyHeader or verifyTx...I know some people want verifyHeader for SPV wallets, but not so sure about verifyTx
< jtimon>
regarding process block, as discussed, I think we should expose verifyBlock instead
< BlueMatt>
jtimon: why, though? if you expose ProcessNewBlock{,Headers} someone can implement a full spv client and full client all in the same codepaths, without exposing low-level partial-verification functions
< BlueMatt>
(or, excuse me, without requiring users do their own utxo-management shit, while still giving them access to query the utxo state)
< jtimon>
how can an SPV node calle ProcessNewBlock?
< BlueMatt>
ProcessNewBlockHeaders
< jtimon>
why would they want to depend on levelDB ?
< BlueMatt>
nono, that part should be abstracted out
< jtimon>
oh, right, you were ok with abstracting storage, just wanted libconsensus to manage reorgs and all the rest in an abstracted way
< BlueMatt>
yea, best to handle /all/ the complexity imo
< jtimon>
isn't ProcessNewBlockHeaders an intermediate function?
< BlueMatt>
no? it takes headers and adds them and selects the best headers chain it has seen
< jtimon>
then I'm not sure what you mean by "intermediate" in this context
< BlueMatt>
ehh, I apologize, I forgot that your definition of verifyBlock also means checking all the txes connect, ie essentially TestBlockValidity
< BlueMatt>
instead of existing CheckBlock-type functions
< jtimon>
well, no really testblockvelidity since that call connectBlock too
< BlueMatt>
hmm?
< jtimon>
not really
< BlueMatt>
huh
< jtimon>
my proposed verifyBlock just tells you whether a block is valid or not, but it doesn't do anything with it
< BlueMatt>
indeed, like TestBlockValidity?
< jtimon>
mhmm, connect block updates transactions, updates undo info and updates the view of the best block, no?
< BlueMatt>
but TestBlockValidity throws away all that info afterwards
< jtimon>
oh, I see
< BlueMatt>
and you have to keep an updated-utxo-state as you connect the block to test validity
< jtimon>
well, then yes, but without generating info than then throws away
< BlueMatt>
you cand check a block without doing that
< jtimon>
yeah, you need to update at least a view of the utxo
< jtimon>
right, that was my idea for verifyBlock
< BlueMatt>
jtimon: ok, so back to my original question...what do you think of not exposing that and actually having a utxo/disk-storage-abstraced full consensus layer
< BlueMatt>
?
< jtimon>
well, where I have te strongest opinion is in abstracting from storage
< jtimon>
and agree on that
< BlueMatt>
ok...?
< BlueMatt>
oh, well sure, I dont think anyone wants libconsensus to require a certain storage layer
< jtimon>
what would be the API? precessnewblock and processnewheaders?
< jtimon>
well, I do think some people want to depend on a concrete storage , but since it's neither of us, let's leave that aside
< jtimon>
what about caches? any API for those?
< BlueMatt>
well once the bitcoin core codebase "eats its own dogfood", as it were, we can take the storage layer we have and expose that optionally
< jtimon>
right
< BlueMatt>
no, caches are up to the client application, ie the client application gives libbitcoinconsensus pcoinsTip
< BlueMatt>
similarly, in the future, maybe we can expose CCoinsViewCache or whatever
< BlueMatt>
but for v1, its uneccessary
< jtimon>
mhmm, I mean for example the versionbits cache
< jtimon>
is that hidden to the caller or exposed somehow?
< BlueMatt>
since its currently not exposed, hidden and present
< BlueMatt>
dont expose anything unless its required, and for v1, dont worry too much about performance or memory usage
< sipa>
the reason for not exposing would exactly be performance and memory usage, i think
< sipa>
it's pretty hard to design a stable api that can be used efficiently for those things
< sipa>
so i'd say make all state (caches, indexes, ...) by default part of the abstracted state
< sipa>
and perhaps later introduce a way for the caller to install callbacks to override it, and provide their own
< jtimon>
well, the api may change if consensus rules change
< jtimon>
BlueMatt: I highly doubt some implementors (say libbitcoin) will use processNewBlock, then again I also doubted they were going to use verifyBlock directly either, I was hoping maybe verifyHeader, verifyTx + GetConsensusFlags
< BlueMatt>
sipa: yea, except for v1 it doesnt matter, really - just dont expose anything and later, if we remove caches to make the library lighter, expose hooks to replace the caches
< jtimon>
leaving that aside, I would like libconsensus to be the specification of the consensus rules for a given block to be valid, it seems that with your approach it would be more than that
< jtimon>
but bitcoin core is currently way more than that, so...
< BlueMatt>
jtimon: I mean the way any sane full node is structured there is an abstracted utxo/blocks-on-disk storage and something which checks everything and itnerfaces with that state
< jtimon>
I also hope you don't intend to put half of the server package in the consensus package
< BlueMatt>
jtimon: I see no reason why libbitcoinconsensus shouldnt repalce everything in between because if the goal is to reduce the risk of consensus incompatibility, we should replace everything we can
< BlueMatt>
jtimon: I dont care what it weighs, only what it does....cutting down the weight is for later versions
< BlueMatt>
:p
< * BlueMatt>
-> brunch
< jtimon>
because some implementors (like libbitcoin) value more the ability to have their own concurrency model (ie without locks) than reducing risks of consensus incompatibility beyond verifyScript (which they also use only optionally)
< * jtimon>
should have brinner soon too
< jtimon>
I agree, that we can leave "cutting weight" for later (like we're leaving moving CFeeRate for later for very long), my point is that with your approach, even after you're done, it will be much bigger than "the specification for whether a block is valid or not"
< jtimon>
it would be more like the specification of "how to follow and store the longest valid chain"
< jtimon>
then someone will want prunning too...
< sipa>
jtimon: yes, it's the specification for whether a blockchain is valid or not
< jtimon>
sipa: processNewBlock does much more than that
< sipa>
well, yes, due to necessity
< sipa>
we can't track the validity of every single chain
< sipa>
so we only have a utxo set at a tip
< jtimon>
that's we, maybe other callers want to do things differently
< jtimon>
then maybe someone else asks for "sharded prunning" or something else, and soon you end up again with a lot of things that don't have anything to do with consensus rules
< sipa>
yes, so we can later provide ways to plug in your own state storage (utxo/caches/indexes/...)
< jtimon>
sipa: oh, BlueMatt's approach already gives you an abstraction for the utxo and chain index storage
< sipa>
oj
< sipa>
ok
< jtimon>
but my example is that if it supports prunning, that needs to be exposed somehow
< sipa>
heh?
< sipa>
how does block storage have anything to do with this
< jtimon>
isn't prunning deleting blocks you are storing?
< sipa>
yes, but blocks wouldn't be stored by libconsensus
< jtimon>
they would with BlueMatt's approach
< sipa>
hmm, that seems strange
< jtimon>
if I undesrtood him correctly
< sipa>
i would have an api where you plug in a way libconsensus to ask you for a block
< sipa>
if yiu're abstracting utxo storage, i don't see why you wouldn't abstract nlock storage
< sipa>
but that's the user that provides the blockstore
< sipa>
not the library
< jtimon>
right, the library provides an API and callbacks the implementor for stored data in both cases, but in bluematt's case, it also updates the storage, it doesn't use it in a read only way
< jtimon>
maybe prunning was a bad example, I agree you don't need to put it in libconsensus even in that case
< gmaxwell>
I think abstracting things like all this storage maybe getting into the realm of imaginary users... is there really many users that want to make no change (not even instrumentation) to consensus but is very interested at implementing their own UTXO database?
< jtimon>
in my case, it takes a block and says valid or validation error x and does nothing more than that
< sipa>
jtimon: but you need to reimplement a lot of complicated logic in your application for it
< sipa>
utxo caching is hard
< sipa>
block index tracking with skiplists is not trivial
< jtimon>
gmaxwell: I agree that part of the problem here is not having a clear idea of the users or their preferences
< gmaxwell>
I thought the goal for libconsensus is that so that when someone wants to make their own custom wallet thing they wouldn't need to reimplment any of the consensus logic, just to create their own application logic.
< jtimon>
gmaxwell: but it seems that your objection would apply to both my approach and matt's
< sipa>
i really don't care about users at this point - we don't have any, and have no easy way to build somwthing others could use
< sipa>
i care about separating consensus from nonconsensus logic
< gmaxwell>
if so that would really mean all the chain management would really be inside the library.
< gmaxwell>
sipa: okay so for the purpose of consensus safty of changes, the caches and utxo database are all consensus critical--
< sipa>
gmaxwell: fair enough, but so is the c++ standard library
< jtimon>
gmaxwell: right, matt puts the chain management inside, but also provides an abstraction for storage
< sipa>
i think having block storage outside the library is perfectly fine, as consensus logic really does not need blocks except for reorgs
< gmaxwell>
jtimon: Providing an abstraction for IO though is different than something that expects the user to implement a complex database.
< sipa>
even utxo storage i can be convinced of, i think
< sipa>
but block chain tracking (cblockindex/mapblockindex/...) should really be inside
< sipa>
it's highly dependent on how we manage chains
< jtimon>
providing a wrapper that includes the same storage as bitcoin core, as an extended library should be easy
< gmaxwell>
so long as the requirements are very clear and simple... where no bitcoin specific understanding is required... sure. though doing that without breaking efficiency might be hard.
< sipa>
i don't want every cblockindex access to go through wrapper calls
< gmaxwell>
e.g. there are ordering requirements for fixation of block data vs utxo data to disk.
< jtimon>
yep, efficiency is the main concern with my approach I think
< jtimon>
and the way I implemented it there, is also inneficient for bitcoin core
< jtimon>
anyway, there's 2 separated topics here
< jtimon>
whether to abstract and expose storage
< jtimon>
and whether or not to manage reorgs and other things beyond "check whether a given block is valid or not"
< jtimon>
my answers are yes and no, matt's is yes and yes. If gmaxwell's is no and yes, and sipa's are yes and yes, we have all the options represented :p
< jtimon>
s/sipa's are no and no/
< sipa>
the first thing we should probably do is separate CBlockIndex into block storage data (file, offset, undo, ...), and chain data (block index, work, validation)
< sipa>
that would be required if we'd want to abstract block storage out from a library
< sipa>
but i thibk it shoukd hapoen regardless
< jtimon>
I don't need to do that in my approach
< sipa>
the block storage data doesn't even need to be in memory permnantly
< sipa>
i understand
< sipa>
but my point is that we should do that, even if there never is a library at all
< jtimon>
chain.o and coins.o are already separated, I'm not entirely sure I understand what you mean
< sipa>
CBlockIndex stores things about where blocks are on disk
< jtimon>
oh, I see
< sipa>
(which has nothing to do with consensus)
< sipa>
and things like validation flags and chainwork
< sipa>
and the block header
< sipa>
which do
< jtimon>
regarding the validation flags, I replaced #7779 with https://github.com/bitcoin/bitcoin/pull/9271 (although got errors in unittests because we're already relying on the consensus flags being coupled for testing it seems)
< sdaftuar>
gmaxwell: regarding bumpfee... i'd been advising mrbandrews to try to simplify what the rpc command is doing as much as possible, with the hope of getting a helpful utility in place tht we could layer more advanced behavior on top of
< sdaftuar>
gmaxwell: so in particular, i wasn't aware of a robust way to identify the change output, so i suggested for this rpc to have the user supply it
< sdaftuar>
is there a robust way to identify the change output?
< sipa>
yes, IsMine() and not in address book
< sdaftuar>
sorry i'm so unfamiliar with this... i guess there's an IsChange() function, with a TODO saying that this might not work with multisig behavior in the future. is that not a concern now?
< gmaxwell>
sdaftuar: We really should be storing the data in the wallet, but just using IsMine would be better than asking for it.
< gmaxwell>
(I think specifying it is basically something that would only be useful to the same people who could use createrawtransaction)
< gmaxwell>
Simplifying it is good, but we can't make it a hazard to users-- which is why I was saying it needed to somehow assure that the outputs will not get spent while unconfirmed.
< gmaxwell>
Simiarly, just diminishing the change and never adding inputs will produce transactions which won't relay (dust). though I suppose instead of being able to add inputs it could just limit itself to places where that wouldn't be the case.
< sdaftuar>
gmaxwell: i haven't looked at the PR lately but i thought it was going to do the right thing with dust.
< gmaxwell>
Because of the need to restrict dependant transaction chains I thought a 'bump everything' might be simpler.
< gmaxwell>
sdaftuar: it didn't look like it could add inputs to me but equally possible that I was confused.
< sdaftuar>
gmaxwell: oh it won't currently add inputs, as i recall, but i think if it would reduce the change output to less than dust, then it would get dropped altogether
< sdaftuar>
(that's what i recall some discussion of, anyway)
< sdaftuar>
are you saying though that you don't think it should allow feebumping a tx that has descendants?
< sdaftuar>
i thought that was part of the point of this RPC, to get that right.
< gmaxwell>
imagine you have txn A, and B that spends A. Then you bump A creating a replaceme A' that replaces A/B. Now you make nother spend that spends the output of A' ... you really need to draft two versions of it one that spends B and one that spends A'... and switch to relaying the other depending on what confirms.
< gmaxwell>
I thought that was too advanced, which is why I was suggesting prohibiting descendants.
< sdaftuar>
ahh right
< sdaftuar>
wow, i totally missed that
< gmaxwell>
GAit has implemented bumping in the green address wallet and I think it deals with all this stuff.
< sdaftuar>
PRs welcome? :)
< gmaxwell>
even my suggestion of making unspendable for one confirm is a bit risky, since it couple be reorged out. :(
< sdaftuar>
so i think bumpunconfirmed is a good idea, but i worry about old transactions that have long since been double spent
< gmaxwell>
esp since there is non-trivial amounts of hashpower that won't take replacements.
< sdaftuar>
or rather, distinguishing those from other unconfirmed transactions
< gmaxwell>
well we track conflicted transactions.
< sdaftuar>
but not perfectly
< sdaftuar>
we can't track conflicted transactions where the conflict chain is broken with tx's not in our wallet
< gmaxwell>
Point. Though it would be sufficient for this functionality to only work on transactions where the closure of unconfirmed parents are all in the wallet, I guess.
< gmaxwell>
I wonder how often someone has multiple unconfirmeds pending and doesn't want to just bump them all. Usually it will save them total fees if they do so.
< gmaxwell>
ignoring crazy corner cases like some being doublespent.
< sdaftuar>
yeah i think it does seem like a useful feature
< gmaxwell>
it just seemed simpler to me than getting all the corner cases right.
< sdaftuar>
well i think that has a bunch of corner cases too. you probably want to make sure you conflict with anything that might possibly confirm, so you have to include even abandoned transactions right?
< sdaftuar>
(maybe even 1-confirm tx's!)
< sdaftuar>
er 1-conflicted
< gmaxwell>
You need to conflict with anything whos outputs you include, so that you don't potentially double-pay.
< sdaftuar>
i guess that raises a good UI point
< sdaftuar>
probably the user would want to see all the outputs and transactions being bumped, and ack that?
< sdaftuar>
in case of the really old wallet tx that the user forgot about problem
< sdaftuar>
i dunno, i guess i just assume that heavy wallet users that have been running for a while must have lots of those, maybe that's not a good assumption
< sdaftuar>
?
< gmaxwell>
I think it would work like this: take the set of unconfirmed transactions which are AllFromMe (whatever the test that excludes coinjoins is called). Then eliminate all transactions whos parents aren't either all AllFromMe or 6 confirmed. Then gather up all their outputs, eliminate all the IsMine outputs, and generate a new transaction which conflicts each transaction in the set, and pays suffi
< gmaxwell>
ciently more fees. Then its change output (if any) should be marked so that it will not be treated as IsFromMe for spending purposes. (UI wise we should encourage somewhat overpaying due to this last point)
< gmaxwell>
I don't think most users have a lot of long lost transactions. I hope not. :) since they'll pretty likely to lose coins if they ditch the wallet thinking the balance was zero.
< sdaftuar>
that seems like it ought to work.
< sdaftuar>
do we have a way right now to do the mark-change-as-unspendable thing?
< gmaxwell>
We have something that doesn't do what we want here.
< gmaxwell>
(lockunspent)
< gmaxwell>
For this, I think we need to add a boolen to the wallet txn and have isfromme check that.
< gmaxwell>
Later we could have more advanced bumping logic that does the make-multiple-transaction-versions.
< gmaxwell>
sdaftuar: if you really think that there are many users with txn stuck never confirmed... then we really need to add a 'Pending Payments' -- sum of confirmed outputs which are spent by unconfirmed transactions -- in the UI/rpc. So that people don't discard wallets as empty when they're really not.
< sdaftuar>
gmaxwell: actually, maybe i am missing something. with your proposed algorithm, what would happen if you bumpunconfirmed twice?
< gmaxwell>
You would ignore the existing bump (it would fail the AllFrom me in step 1) and make a new bump of all the other unconfirmed transactions in your wallet.
< sdaftuar>
i didn't follow why it would fail the AllFromMe step, but if you managed to construct a new tx that didn't conflict with the older bumped fee one, you're screwed right?
< gmaxwell>
It would fail the all from me, because I propose the change output of the bumb be flagged specifically so that it does. ... so that under no condition wall the wallet spend it until it is 6 confirmed. Specfically to avoid having to deal with things spending it then having the originals confirm.
< sdaftuar>
isn't IsAllFromMe purely a function of the tx inputs?
< gmaxwell>
The new tx doesn't need to conflict with it, so long as it doesn't pay to any of its outputs, which it won't because it wasn't included in the set of available txn.
< sdaftuar>
i think i am pretty confused right now, why don't i spend another day thinking about the wallet and then rejoin this conversation
< gmaxwell>
It's a function of the input's outputs. Is every input an output of mine.
< gmaxwell>
sdaftuar: this stuff makes my head hurt too. I really want to get some kind of bumping in ASAP, it's hard to come up with a minimal feature...
< sdaftuar>
one last try before i call it a night -- let's say i have tx1, tx2, tx3 -- each one is IsAllFromMe. i bump and create tx4, which conflicts with each, and pays all their outputs, and the change output of tx4 is specially flagged or whatever to be unspendable until it confirms.
< sdaftuar>
wont't tx4 still be IsAllFromMe?
< sdaftuar>
(actually i have to run, back later)
< gmaxwell>
Right, okay -- I should have said IsMine and IsAllFromMe -- the flagging should make it not IsMine.
< gmaxwell>
sdaftuar: ttyl
< BlueMatt>
gmaxwell: ok, I missed a bunch of scrollback, but hell yes...everyone wants the utxo db in their own format so that they can do other queries against it
< BlueMatt>
jtimon: no, I absolutely think we should abstract out block storage
< BlueMatt>
jtimon: hell, the branch I sent you did that for ConnectBlock/DisconnectBlock in the first commit