< tulip>
phantomcircuit: various religions have plans for what happens during the return of a prophet. perhaps we need a list of questions for Satoshi about coding oddities in wxBitcoin, just in case.
< phantomcircuit>
wumpus, am i crazy of will CDB::Flush call bitdb.dbenv->txn_checkpoint(0,0,0) whenever fReadOnly is false
< phantomcircuit>
dblogsize is ignored unless we're in read only mode
< phantomcircuit>
wat
< phantomcircuit>
lol no wonder this is so slow it's not flushing to disk it's reading the log files in database/* and writing them to wallet.dat
< phantomcircuit>
i wonder if that big warning means you can actually correct the database or if you will potentially lose write between write and sync calls
< phantomcircuit>
im guessing the latter
< phantomcircuit>
gmaxwell, ^
< dcousens>
gmaxwell: I'm thinking a ZMQ notification for mempool expiry would be nice
< dcousens>
its kind of a hard event to catch without dumping the entire mempool list otherwise
< jonasschnelli>
wumpus: if you provide / pastebin your bitcoin-win-0.12-build.assert somewhere I can check the packages.
< jonasschnelli>
even the bitcoin-0.11.99.tar.gz doesn't match (against the hashes you have posted)
< jonasschnelli>
i only compared against your hashes, not against another build
< wumpus>
if you upload the files, I'll compare them
< wumpus>
the source archive doesn't match either? that's (ruling out tar nondeterminism issues) almost certain indication that it was the wrong source code that was built :)
< jonasschnelli>
wumpus: yeah.. i have though this also. But: git:b08293544a207088193de8834bb754f5d212c9bf bitcoin
< jonasschnelli>
Can you compare: b8affff612d645598a4642dcc4eef7d4974c02d73c31a99ba2faa36425142aca bitcoin-win-0.12-desc.yml?
< wumpus>
so when I follow your link I see
< wumpus>
67c3bc85ece1ad2905a7f801277fbd76d1e7ac653ad2e021cd7fadf1fa6a6307 src/bitcoin-0.11.99.tar.gz MATCH
< wumpus>
7fd5e22a794a6fa34defe0cd0e82d8f0ad759fba73e190aa5bd202627fa45bc5 bitcoin-0.11.99-win-unsigned.tar.gz MATCH
< wumpus>
they all match to my hashes
< jonasschnelli>
grml...
< jonasschnelli>
right. They match. I compared against the wrong file...
< wumpus>
I don't have the yml anymore so can't check that one :(
< wumpus>
but it's just the hash of the hashes, so should be the same
< jonasschnelli>
Nice to see this working!
< wumpus>
let's hear from fanquake if he gets the same
< jonasschnelli>
paveljanik: "Maybe his screenshot is from the same code from which you did the screenshot." -> could be, but github does not show a push in between (chronologically)
< paveljanik>
yes
< paveljanik>
But I think it is RtM now
< wumpus>
yes, would make sense to make a custom out of providing the current HEAD commit that our comments are about
< Luke-Jr>
wumpus: wait, you have gitian integrated in your browser?
< wumpus>
I don't think so :)
< Luke-Jr>
so the "MATCH" wasn't part of what you saw there? aww
< wumpus>
no I added that part myself - yea, disappointing :)
< MarcoFalke>
wumpus, when will translations close?
< wumpus>
translations never close, there will be a string freeze after Dec 1 though, to prevent translation strings being changed in the source code and thus giving translators time to catch up
< wumpus>
translations from transifex will be merged up until the last rc
< MarcoFalke>
ok
< GitHub9>
[bitcoin] laanwj opened pull request #7060: doc: Make networking work inside builder in gitian-building.md (master...2015_11_gitian_building) https://github.com/bitcoin/bitcoin/pull/7060
< MarcoFalke>
wumpus, about the "all fees in bitcoin core are per kB" thing:
< MarcoFalke>
Do you think 6708 should go into .12?
< wumpus>
MarcoFalke: I think so, but needs more review/testing
< * jonasschnelli>
wonder what why we have the "Pay only the required fee of 0.00001000 BTC/kB" in the UI
< wumpus>
that's the -mintxfee I think?
< jonasschnelli>
std::max(mintxfee, minrelayfee)
< jonasschnelli>
but it's somehow wrong. "required fee" is a misleading term.
< wumpus>
I agree that it is misleading
< wumpus>
'required' by whom, for what. It no longer makes sense now that all fee thresholds are going dynamic
< jonasschnelli>
And one line above, you can select fee: [ ] per Kilobyte [ ] total at least [____ amount ___]?
< jonasschnelli>
So somehow i think we should entirely remove fPayAtLeastCustomFee together with the GUI line.
< MarcoFalke>
> 'required' by whom
< MarcoFalke>
Required by mempool and the node operator
< MarcoFalke>
GetMinimumFee() however takes also into account current network conditions
< jonasschnelli>
I think either you go for the "Recommended" fee (estimation) or you choose a custom fee (per KB or absolute).
< jonasschnelli>
Even there, does an absolute fee make sense?
< wumpus>
absolute makes no sense
< jonasschnelli>
What if your transaction has 20 ins and outs?
< wumpus>
needs to be per kB
< wumpus>
but I agree that you either want bitcoin core to choose a fee for you (estimate confirm within # confirmations) or you want to set a fee/kB, which should be above what your mempool accepts at all
< MarcoFalke>
I think the only reason we have this is that there were some lazy unit test writers
< jonasschnelli>
the software should not follow the unit/rpc tests. Should be the other way around. :)
< MarcoFalke>
"Let's just assume every transaction is 1000 bytes and less, so we can hard code the balance asserts"
< jonasschnelli>
I agree that test with fees are difficult and break easily, but this can't be a reason to not make changes.
< MarcoFalke>
* Forgot to eat lunch, gone now.
< jonasschnelli>
hmm... maybe users will complain that they can set an absolute fee (if we remove that feature).
< jonasschnelli>
If absolute fees are possible, they should be hidden behind CoinControl feature (or similar expert setting)
< wumpus>
they can't set an absolute fee can they?
< wumpus>
you can't know the size of the transaction in advance so what point is it?
< wumpus>
it'd require you to second-guess the input selection knapsack algorithm
< jonasschnelli>
The ui says custom fee [ ] --- [ ] per KB , [ ] total at least ______ <- (amount)
< * jonasschnelli>
is checking the code
< wumpus>
(exeapt in the case of coin control where you choose inputs yourself, but in that case you know the ~size of the transaction so you could just as well set per kB)
< wumpus>
jonasschnelli: crazy! never really paid attention to that
< jonasschnelli>
wumpus: me too ... :)
< sipa>
jonasschnelli: i find strprintf far more readable generally than "a" + b + "c" :)
< sipa>
(and that's all i'll say about the topic)
< jonasschnelli>
sipa: Yes. It's better readable. I Agree,... i don't want to microoptimize that stuff, but i normally try to avoid printf when it's just appending strings.
< jonasschnelli>
But probably only matters if your on an embedded system and calling the printf a LOT
< sipa>
a + b + c may actually be less efficient than strprintf("%s%s%s") for large strings, as the first operation needs to allocate a new string twice, and copy the contents of a 2 times
< wumpus>
yeah... and in that case you probably don't want to be processing strings in the critical path a t all
< sipa>
and that ^
< jonasschnelli>
agreed. For sure it's okay in the Luke-Jr PR. I regret bringing up this point... :)
< wumpus>
you're right that strprintf isn't terribly efficient, its goals are compatibility with sprintf (which was used before, so there wasn't impact on all debug messages) and type-safety
< sipa>
i've never benchmarked it :)
< wumpus>
tinyformat's github page has some benchmarks
< wumpus>
but yeah if its performance matters you're doing something wrong :)
< sipa>
agree there
< jonasschnelli>
"but yeah if its performance matters you're doing something wrong" <- that's a good point
< wumpus>
(e.g. LogPrint has a shortcut to avoid calling tinyformat at all if the message is not being logged)
< instagibbs>
wumpus, can we confirm user-oriented scripts go in 'share'? I can't glean the intent of the folder by its contents unfortunately
< instagibbs>
there are some images, certs, etc
< wumpus>
instagibbs: to be really sure you should ask cfields_
< instagibbs>
cfields_, *ping*
< wumpus>
user oriented scripts need to be installed, which means they should go into a directory that's normally part of the tarball, as well as added to the 'make install'
< wumpus>
which reminds me, the man pages need that too
< wumpus>
contrib/ is for examples and such, as well as things only necessary during development
< morcos>
wumpus: if we are to have a string freeze by Dec 1st. We really need to solve the UI problem of how to report to user and deal with txs that have been evicted from the mempool.
< morcos>
dgenr8 had some comments on 6871 on what this functionality should look like. I think we should probalby try to agree on that design, and hopefully it won't be too hard to implement.
< morcos>
In particular, if you have a non-optin-for-replacement tx that has been removed from your mempool. Does it make sense for their to be an option to manually release the inputs for double spending? It seems relatively likely to me that just rebroadcasting the original tx isn't going to be that helpful unless you can send it directly to a miner who will prioritize
< morcos>
However if we want to keep the closest to the existing behavior, then perhaps we do not allow that if you've not opted in for replacement.
< morcos>
existing behavior I mean 0.11. right now master will just let you respend it anyway. but i think having it evicted from your mempool in 0.12 is the same thing as just having it sit forever at the bottom of your mempool in 0.11 which doesn't allow you to respend
< morcos>
In the case that you have opted in for replacement, I suppose the question is if you want to replace it and its been evicted, are you stil required to follow the replacement rules? This would be a lot of wallet code that hasn't been written.
< morcos>
So I guess the answer to that is no. Actually the wallet is not aware of whether you have opted in for replacement or not.
< morcos>
Anyway, the point is lets sketch out what the desired behavior is so someone can implement it. But master as it exists now is a bit of a mess UI wise
< morcos>
sipa: do you think we should have some kind of configuration warning/check if someone had a config file with maxsigcachesize=100000 or something
< wumpus>
morcos: a related problem is that with -walletbroadcast=0, transactions never enter the mempool at all
< wumpus>
(well that's not a problem in itself, it's what it is supposed to be , but how it is shown is suboptimal)
< morcos>
wumpus: hmm.. i wasn't aware of that, so do they also show as conflicted?
< wumpus>
es
< wumpus>
+y
< morcos>
hmm.. putting aside the timline for how fast all this will be fixed
< morcos>
what is the ideal behavior there
< morcos>
is there a reason we don't put it in the mempool and just don't relay it?
< wumpus>
privacy problems - you'll never request it
< morcos>
ah, we're waiting for it to be relayed back to us?
< wumpus>
the point of walletbroadcast=0 is to completely isolate the wallet from the node in one direction
< morcos>
what do you mean you'll never request it? oh, right, what i jsut said
< morcos>
hmm... ok, so in that case you'd want to ignore a child tx to that tx anyway until you saw the parent
< wumpus>
also putting it into our own mempool can be a leak as the mempool can be requested through P2P
< wumpus>
the ideal behavior there would be to just trust that a transaction that you created yourself exists
< wumpus>
e.g. 'created but not yet seen on network'
< wumpus>
entirely ideally there would be a way to delete it again, if you decide not to broadcast it at all
< morcos>
yeah i suppose, but then you get into the question of whether you want to predict whether it had been evicted or not?
< morcos>
i suppose the simple version of that is you expect to see it within a certain amount of time as you tell the wallet it should be on the network now
< wumpus>
I don't think that really matters in the walletbroadcast=0 case, it assumes that the user will manage the transaction and somehow getting it to the network/miner themselves
< wumpus>
the client does not need to care about it anymore, it will either eventually see it relayed on the network, or see it appear in a block
< morcos>
Yes I think the real questions is when to mark inputs as freed for respending by automatic coin selection
< wumpus>
doesn't need to be within a certain time either - it could have been submitted to a low-latency relay network that takes hours to release it somewhere
< wumpus>
it shouldn't
< morcos>
So right now when a tx is not in your mempool, they are marked as freed correct?
< wumpus>
I think the behavior where a transaction exists but the inputs are reused is very strange
< morcos>
(I'm talking about code I haven't looked at here btw)
< morcos>
I think if we change the default to be that inputs are not marked as freed just b/c a tx does not appear in your mempool, and add a manual method of saying forget this tx, and ideally add detection for true conflicts which will also free the remaining unspent outputs
< wumpus>
yes, but I think that's inadvertent. The -1 confirm handling was introduced when there were malleablity attacks, to prevent *conflicted* transactions from being counted
< morcos>
Then maybe that covers all cases, and we just change the UI to say something other than conflicted except in the conflicted case
< wumpus>
I'd love a manual way to say 'forget this tx'
< wumpus>
currently the only way is to do -zapwallettxes on the command line which takes a long time as it does a rescan
< wumpus>
but I think being allowed to delete unconfirmed transactions (certainly those that are not in the mempool) wouldn't hurt. There was a previous implementation but I think it was broken in case of dependent transactions.
< wumpus>
sounds good
< morcos>
Heh, I wasn't volunteering. Actually I wouldn't mind, but I'm not going to be around.
< morcos>
But maybe we can point a handy volunteer to this little converation.
< wumpus>
it was slated for 0.10 but there were too many issues with the implementation and no one picked it up again
< jgarzik>
A lot of people would love a 'forget this tx' +100
< sipa>
can we just start by remembering in WalletTx whether accepttomempool failed
< sipa>
(because conflict)
< sipa>
if so, mark it as conflicted and spemdable again
< morcos>
What I think we really need to do is set a minimum viable product for 0.12.
< morcos>
sipa: isn't that what happens now?
< sipa>
if no, it's just 0 confirmed, not -1 confirmed
< sipa>
morcos: no, now we check whether it is in the mempool, and if not, comsider it spendable
< sipa>
(afaik, i haven't looked at the code in a long time)
< morcos>
no -1, just tried it
< morcos>
oh i misunderstood
< sipa>
i thought that not in mempool == -1 comfirms == conflicted == respendable
< sipa>
if not, you should ignore me
< wumpus>
the wallet code gives me a headache. I tried to explain https://github.com/bitcoin/bitcoin/issues/7054 (Difference in getbalance and sum(listtransactions) amounts (testnet)) but failed
< wumpus>
yes I think that's how it is sipa
< morcos>
yes, that is what happens, what are you saying should happen
< sipa>
morcos: make accepttomempool return whether all inputs were available or not
< sipa>
morcos: if not, remember in the wallet that it should be considered conflicted/resoendable
< sipa>
otherwise, just unconfirmed
< morcos>
ok, but that doesn't make sense to me
< morcos>
so it doesn't make it in b/c too low fee
< morcos>
you don't want it to be respendable?
< morcos>
i agree it should say something other than conflicted
< morcos>
but the respendability should be the same if it never made it in in my opinion
< morcos>
if it was in, and got evicted, then i think thats a different question, then it should not automatically be respendable
< morcos>
or if you never tried to submit it
< sipa>
right, if it wasn't accepted at all (and never broadcast) it should be respendable too
< sipa>
but it's very hard to know whether it jever made it to the network
< sipa>
peoppe may manually rebroadcast
< morcos>
but if we're going to implement those features where now sometimes its not automatically respendable, we probably have to implment the forget function
< morcos>
so what do we think is a reasonable goal for 0.12?
< sipa>
a remove function for unconfirmed transactions, and no longer looking at mempool to dexide conflictedness
< wumpus>
+1 sipa
< jgarzik>
+1 + mention @ meeting
< sipa>
+1 jgarzik
< wumpus>
though the feature freeze for 0.12 is in less than two weeks, so there is not much time
< jgarzik>
The remove function will be very popular with users
< sipa>
having the feature freeze so close to hongkong is inconvenient
< morcos>
yeah sorry if i'm being a bit insistent on this, but the impending feature freeze is why i keep bringing this up.
< wumpus>
yes, in general december is always an inconvenient month
< sipa>
oh, we already have *pfMissingInputs in AcceptToMemoryPool
< wumpus>
I don't want to plan the feature freeze during holidays either
< sipa>
we could just remember that result
< morcos>
sipa: so what happens if a tx is evicted
< sipa>
morcos: it remain unconfirmed, but its accepttomemorypool succeeded, so we assume it can still confirm
< wumpus>
then again if you label this as a 'bugfix' it can go in after the feature freeze
< morcos>
ok, does it indicate somehow its not in the memory pool?
< morcos>
i agree, this seems like the right behavior
< sipa>
morcos: so whether a tx is in the mempool isn't relevant; whether it ever made it in is
< morcos>
i understand for whehter or not its respendable
< sipa>
or rather, whether it failed to add it _due to missing inputs_
< morcos>
but to indicate whether or not you might want to manually forget it
< sipa>
the removetransaction functionality can still look at the mempool in the GUI and warn if it's there ("warning: it looks like this transaction may still confirm")
< morcos>
sipa: see that last statement confuses me a bit. I agree we want to distinguish on the reason it failed to add. but if it failed to add, it should still be automatically replaceable in my mind
< wumpus>
sipa: but only if it was requested by at least one other node :-)
< sipa>
morcos: what if you received it from the network in the first place, but didn't get added to the mempool
< morcos>
sipa: what happens now in that case? are you even alerted to the txs existence
< sipa>
i don't think so
< morcos>
so no change there then
< wumpus>
transactions not accepted into the mempool don't exist from the viewpoint of the node
< wumpus>
(if you receive them)
< wumpus>
this prevents some wallet spam attacks
< sipa>
fair enough
< morcos>
ok , i agree though. a good baseline is to not conflict unless its conflicted. so non-conflicted non mempool txs wont' be auto respendable. but you can manually forget them. its an optimization as to whether you want to detect that it never made it into the mempool period, but we dont' need it, i was being too picky.
< sipa>
i like that
< morcos>
in other news, sorry for all the churn on 6898, i'm trying to get it into as final a state as possible, and did a bunch of testing yesterday.
< morcos>
sdaftuar is reworking the index to allow for considering manual tx prioritisation for eviction
< morcos>
if there are no objections i will rebase and squash once more off that commit instead of my index commit
< morcos>
sipa, should the dynamic memory usage now be 12 * ptrs with 4 indices?
< sipa>
morcos: let me have a look at the boost code to be sure :)
< sipa>
ordered indexes are 3 pointers overhead
< morcos>
great thank you
< sipa>
hash indexes are harder
< jgarzik>
morcos, RE manual tx pro for eviction - dumb question - what does that imply for the goal of removing tx prio from mempool?
< sipa>
jgarzik: right now priority-based things in the mempool are very easily evicted
< morcos>
jgarzik: i was trying to distinguish between 2 overly similar names
< morcos>
i'm talking about adding a feeDelta using prioritiseTransaction
< jgarzik>
ah, ok. That I understand. :)
< morcos>
i don't think we're talking about removing that, although of course getting rid of priorityDelta would be nice
< jgarzik>
morcos, RE feeDelta... ok, that I understood. tnx
< jgarzik>
That's pretty much how my original remove-prio patch worked.
< jgarzik>
feeDelta could be used to execute arbitrary policy
< sipa>
jgarzik: it seems that that is not really the case; you can come up with a formula to treat bitcoin-days-destroyed as extra fee, but it's probably hard to prevent it from making miners lose large amounts in fees
< sipa>
(though if someone succeeds in that, I still like the idea!)
< jgarzik>
sipa, nod - my idea was keep the current two-pass system
< jgarzik>
sipa, if transactions fall out of pass #1, apply deltas, try again, see what happens
< jgarzik>
check reality first, then distorted reality
< sipa>
hmm
< sipa>
but that's just for mempool eviction
< sipa>
you need it also in block construction
< morcos>
sipa: jgarzik: Just to sanity check what sdaftuar and I are doing, we are making any feeDeltas you put on transactions through prioritiseTransaction be treated just like real fee.
< sipa>
sounds good to me
< morcos>
so for instance if you add a big feeDelta to every tx, your mempool will end up with a very high minimum fee
< morcos>
i can't think of any other way that would make sense, but just wanted to be sure
< jgarzik>
morcos, yes
< morcos>
of course the one exception is calculating the coinbase in the mining code (i hope)
< sipa>
morcos: i think you should literally treat it as "I'm being paid out of band for this tx"
< morcos>
once we remove priorityDelta for 0.13... we can change mapDeltas to only exist for txs not yet in the mempool i think...
< morcos>
sipa: yes it just want clear to me that people knew to think about the scale, because before a limited mempool, it wouldn't really matter if you added huge fees to everything.. you'd just try to mine them first
< morcos>
wasnt
< btcdrak>
sipa: Thank you very much for that patch for #6312 I hadnt quite grasped what you originally meant.
< sipa>
btcdrak: oh, ok!
< GitHub36>
[bitcoin] sdaftuar opened pull request #7062: [Mempool] Fix mempool limiting for PrioritiseTransaction (master...fix-mempool-limiting) https://github.com/bitcoin/bitcoin/pull/7062
< cfields_>
instagibbs: pong
< instagibbs>
how shall I include a python script that is intended for end-users of Core? Which directory, etc?
< GitHub67>
[bitcoin] sdaftuar opened pull request #7063: [Tests] Add prioritisetransaction RPC test (master...add-prioritisetransaction-rpctest) https://github.com/bitcoin/bitcoin/pull/7063
< sipa>
hmm, do we have any means for people to select pruning at first run of Bitcoin-Qt?
< jtimon>
code itself and the code relying on it is stable, and it is clear which
< jtimon>
parts are configurable and which aren't."
< sipa>
jtimon: yes; that PR itself may be fine, but it can't really do much useful in terms of encapsulation
< jtimon>
sipa: well I can't open the ones that depend on it until it is merged without creating a lot of noise
< sipa>
so just wait until all this mempool stuff has cooled down :)
< jtimon>
#6423 #5114 #6420 and much more stuff is waiting for #6068
< jtimon>
sipa: but I strugle to understand why one thing needs to wait for the other
< jtimon>
if #6068 won't interfere with "the mempool stuff", why does it have to wait?
< sipa>
if you want to do any useful encapsulation of policy it does
< jtimon>
because #6423 #5114 #6420 are useless in terms of encapsulation or interfere with the mempool stuff?
< jtimon>
when the mempool code is "ready" for me to be able to encapsulate that part, I will still want to do this stuff first
< sipa>
5114 seems perfectly fine and independent of any policy classes
< jtimon>
well, #6420 wasn't a great example, but most of my policy branches weren't opened as PRs
< sipa>
oh, it does add that too
< jtimon>
5114 is waiting for #6068 because it could be a method in CPolicy
< jtimon>
waiting for ages, long before any of the "mempool stuff"
< jtimon>
and I think #6423 could have been a model for adding new mempool attributes/globals
< jtimon>
of course the code in the PR is heaavily out of date
< jtimon>
but I have newer versions of all that stuff. some more advanced things [ie fully decouple txmempool and policy/fees from each other, you may not believe this] are somewhere in the pile of forgotten branches...
< jtimon>
and I won't try to repeat that until the mempool code is more stable
< jtimon>
but the things that I know can only conflict trivially...
< morcos>
jtimon: what happened to your high level document
< morcos>
i'm all in favor of getting some of these changes merged, b/c its getting hard to write new code and access the right things
< sipa>
morcos: that was about consensus stuff movement, not policy encapsulation
< morcos>
i think we should have a time shortly after 0.12 is branched or released, i don't care which, in which we actively merge a lot of refactors
< morcos>
sipa: in my mind it was about overall code architecture
< morcos>
i would be happy to rework any open pulls i have then and help review
< jtimon>
morcos: that's for libconsensus, not policy
< jtimon>
morcos: but yeah I should go back to that
< jtimon>
which reminds me...I need to ask cfields what would he think about a consensus building module separated from util, common, etc, maybe merging with libsecp256 and crypto modules (that is not good for bitcoin-tx when we start adding non-tx stuff to the module, but verifyheader and verifyblock should be relatively light compared to verifytx and verifyscript)
< jtimon>
anyway, I should adapt it to post-libsecp256k1, it should make the document simpler and clearer, and change it to plan that starts after forking 0.12 instead of right before it
< jtimon>
s/plan/a plan/
< instagibbs>
cfields_, ping: how shall I include a python script that is intended for end-users of Core? Which directory, etc?
< dcousens>
wumpus: what are your thoughts on using github milestones for tracking what issues/PRs need to be resolved for releases (say, 0.12)
< dcousens>
I feel like that'd be really useful to help concentrate review effort etc
< sdaftuar>
dcousens: there are PRs tagged for 0.12
< dcousens>
sdaftuar: I don't see the 0.12 label?
< sdaftuar>
it's under milestones
< dcousens>
right, its already being done
< cfields_>
instagibbs: depends on what it is, i guess
< cfields_>
jtimon: not sure what you mean
< dcousens>
sweet, nvm :)
< dcousens>
sdaftuar: just didn't see a single recently updated PR with a milestone, and assumed otherwise haha
< jtimon>
cfields_: so what's in libbitcoinconsensus_la_SOURCES = \
< sdaftuar>
i'll take that as an opportunity to beg for review -- please take a look at #6494! :)
< jtimon>
is repeated in libbitcoin_util_a_SOURCES, libbitcoin_common_a_SOURCES, etc
< instagibbs>
it has a script that is basically the suggested way of generating a name/salt/pass
< jtimon>
cfields_: we could have a libbitcoin_consensus_a_SOURCES that libbitcoinconsensus_la_SOURCES takes but it's also build separately as part of building bitcoind (like util, univalue, etx)
< cfields_>
instagibbs: mm, share/ would probably be the best fit
< jtimon>
s/etx/etc
< cfields_>
jtimon: i have a branch around somewhere that does something similar
< cfields_>
jtimon: problem is the mixing of libtool/non-libtool
< jtimon>
cfields_: does that make sense to you? at some point we need to do that AND move all the code inside the same folder if we want to make a subtree ala https://github.com/libbitcoin/libbitcoin-consensus
< instagibbs>
cheers, I'll move it around.
< jtimon>
cfields_: it would be great if you could resurrect it
< cfields_>
jtimon: yea. i believe the issue was that libtool doesn't like linking a lib into another lib. Can't remember if i PR'd changes to fix that behavior or not
< jtimon>
cfields_: I'm not sure I undesrtand the libtool/non-libtool but things like script/interpreter.cpp not appearing twice seems trivial
< cfields_>
jtimon: go for it :)
< jtimon>
cfields_: well, probably that's the correct(tm) solution but I think something simpler could be done: libbitcoinconsensus_la_SOURCES just happens to build the same things as the libbitcoin_consensus_a_SOURCES module, but they still build twice
< jtimon>
cfields_: well, I don't have much experience with the building part
< jtimon>
I just see a util and a common module and want another one for consensus
< cfields_>
to what end?
< jtimon>
I mean, if you're willing to point out my mistakes I may try to do it myself
< jtimon>
at some point we want to have it in an independent repo, no?
< jtimon>
therefore it seems a logical module to be built together (like libsecp256 currently is)
< jtimon>
I mean, that's what I'm asking if makes sense to you
< jtimon>
it could even include crypto and libsecp256 in one single module (independent from util and common)
< cfields_>
jtimon: that makes sense to me (not sure about making external), but imo it's not worth shuffling around the build stuff until the end
< jtimon>
cfields_: well, I think I disagree, I want the compiler to tell people when they "un-encapsulate" or "add unwanted dependencies" to consensus code
< jtimon>
for example, if I move primitives/block.o to that module and then someone includes util.h in block.cpp, the linking should fail
< cfields_>
jtimon: in that case, build/link would still succeed if it's pulling header-only stuff from util.h. sounds like you want to be messing with include paths instead.
< jtimon>
IMO, it helps to more easily "protect" the already-encapsulated code, like moving the files to the same folder, it can always be done later, but it makes things clearer to devs I think
< jtimon>
at least not if is something defined in util.cpp, right?
< cfields_>
right
< jtimon>
something is something, for the other thing I guess we would need to divide BITCOIN_CORE_H or something, but seems much more messy if it's even possible
< cfields_>
it'd likely involve moving non-lib-safe headers to their own dir, yea
< jtimon>
that seems like a smart division
< jtimon>
so do you think you could do the consensus module?
< jtimon>
I mean...are you interested?
< cfields_>
jtimon: not at the moment, i'm trying to get net stuff firmed up
< jtimon>
btw, right now is a mess that I don't want to even show you, but if you want to co-author of the libconsensus planning doc, you're more than welcomed, last time I just happened to be breaking up script and learn a lot from you BlueMatt and sipa (also invited to become co-authors), at least I hope all of you will find time to review it
< jtimon>
cfields_: mhmm, ok, I guess I can try it myself and nagg you to point out my mistakes, do I have to touch any other file apart from src/Makefile.am ?
< cfields_>
jtimon: sure, i'll have a look. I'd like to help out, but i'm currently overcommitted and afraid i'd just slow you down
< cfields_>
shouldn't
< jtimon>
cfields_: ok, in any case the plan will start after forking 0.12 so no rush to publish it until that happens
< cfields_>
ok
< jtimon>
cfields_: thanks, I'll try it and I may come with more questions
< cfields_>
sounds good :)
< jtimon>
thoughts on unifying the new consensus module with libsecp256 and crypto modules (I believe the current libconsensus depends on the whole crypto module and libsecp256k1 while nothing that depends on those modules doesn't depend on consensus too?) ?
< cfields_>
not sure what you mean by unify...
< cfields_>
keep in mind that consensus isn't the only POV for layout
< jtimon>
what's in libbitcoin_util_a_SOURCES, for example, is linked as the same module/package, right?
< jtimon>
aren't those these the packages/modules in which bitcoin core is divided?
< cfields_>
jtimon: that's what i meant by "consensus isn't the only POV". For (bad) example, bitcoin-tx might need hashing, but not use libsecp256k1
< jtimon>
yeah, is it a problem if it depends on both?
< jtimon>
or rather, how much of a problem it is?
< cfields_>
jtimon: better to keep the chunks small and localized, and let the bigger libs/progs only include what they need
< jtimon>
assuming a future in which bitcoin core consumes libbitcoinconsensus C API directly instead of its code, that dependency will eventually happen
< jtimon>
is that assumption crazy?
< cfields_>
no, but the assumption that everything that needs _some_ parts of the consensus code need _all_ of it is, i think
< jtimon>
that's the assumption I'm talking about
< jtimon>
is not that needs it, but it has to link it as a whole
< cfields_>
then yes, i think that's a bad path
< jtimon>
in the same sense, bitcoin-tx may not require everything in util or common, but it depends on util and common as a whole
< jtimon>
doesn't it?
< cfields_>
yes, but common/util were split up for just that reason
< jtimon>
doesn't bitcoin-tx depend on both of them?
< cfields_>
yes, but see bitcoin-cli
< jtimon>
does bitcoin-tx use every function defined in every cpp in util and common?
< jtimon>
what's with bitcoin-cli ?
< cfields_>
jtimon: no need to take this to the extreme. The lines are somewhat arbitrary and could certainly be redrawn. But i'd rather see the scopes narrowed than grown.
< jtimon>
well, my main goal is consensus being one scope (instead of having hash.o in common and uint256 in util, for example), so let's just forget about the "unifying consensus + crypto + libsecp256" thing for very long
< jtimon>
cfields_: instead of doing a full library-friendly vs non-friendly separation I will only separate the current libconsensus headers for now, which reminds me...where do you think ScriptErrorString() (removing script_error.cpp) should be put?
< jtimon>
it doesn't seem to be needed for libbitcoinconsensus
< cfields_>
jtimon: yea, i suppose you could call that a core-specific thing, since it's not exposed
< cfields_>
though, heh, core doesn't actually use it anywhere
< jtimon>
oh, so we can actually just remove it or is it supposed to be used by libconsensus?
< jtimon>
or by core?
< cfields_>
the original intention was to have api-stable error codes, and that function would be exposed as well
< cfields_>
but once we dropped the external error codes, it stopped being useful for anything other than internal debugging, i guess
< jtimon>
assuming we didn't had dropped them
< jtimon>
where would this errorCodeToString() function go?
< cfields_>
libconsensus
< jtimon>
ok, so then script_error.cpp should be in libbitcoinconsensus_la_SOURCES right?
< cfields_>
yea
< jtimon>
I mean, for example, amount.h is part of libconsensus but amount.cpp isn't (because once we take IsDust out of transaction.h, CFeeRate can go somewhere in the policy folder)
< jtimon>
I thought this could be a similar case, so thanks that's very useful
< phantomcircuit>
the current wallet bdb behavior is to write everything to wallet.dat in CDB::Flush and to only use the log files as crash recovery
< phantomcircuit>
which means there's a basically free 2x speedup available by flushing to the log instead of to the log and the backing store
< phantomcircuit>
the question is are we ok that wallet.dat might also need the database directory for upto 500ms after an operation has completed?
< sipa>
i'm fine with that
< sipa>
i thought that was the model anyway: at shutdown, we try to make sure the log files are empty and the wallet.dat files are independent
< sipa>
but before that time, in case.of a crash, you need the log file
< phantomcircuit>
sipa, nope currently the only time you need the log files is if you actually crash
< phantomcircuit>
also doing it this way reduces the total number of fsync calls as bdb will coalesce writes to wallet.dat
< sipa>
good
< phantomcircuit>
still cant figure out why calling pdb->sync() doesn't cause an actual fsync/fdatasync though
< phantomcircuit>
sipa, actually it's closing the database handle which i believe isn't necessary