< Guest88196>
to accept Islam say that i bear witness that there is no deity worthy of worship except Allah and Muhammad peace be upon him is his slave and messenger
< sipa>
Guest88196: off topic
< cfields>
BlueMatt: let me know if you want me to PR our combined changes, don't want to rip it out from under you
< BlueMatt>
cfields: hummm...feel free
< BlueMatt>
though I'm not a huge fan of that, actually
< BlueMatt>
wellll
< BlueMatt>
hum
< cfields>
BlueMatt: heh, one of these days we'll just agree on something and that will be that :)
< BlueMatt>
i mean it just seems shit to do this
< BlueMatt>
like, we should always be checking something else
< bitcoin-git>
bitcoin/master c735540 Matt Corallo: Move ORPHAN constants from validation.h to net_processing.h
< bitcoin-git>
bitcoin/master edded80 Matt Corallo: Make ATMP optionally return the CTransactionRefs it replaced
< bitcoin-git>
bitcoin/master 1531652 Matt Corallo: Keep shared_ptrs to recently-replaced txn for compact blocks
< bitcoin-git>
[bitcoin] laanwj closed pull request #9499: Use recent-rejects, orphans, and recently-replaced txn for compact-block-reconstruction (master...2016-12-recent-tx-cache-cmpct) https://github.com/bitcoin/bitcoin/pull/9499
< bitcoin-git>
[bitcoin] practicalswift opened pull request #9581: [pep-8] Prefer "foo not in bar" to "not foo in bar" (master...test-for-membership) https://github.com/bitcoin/bitcoin/pull/9581
< bitcoin-git>
[bitcoin] practicalswift opened pull request #9582: [pep-8] Prefer "foo is None" to "foo == None" (master...is-none) https://github.com/bitcoin/bitcoin/pull/9582
< bitcoin-git>
bitcoin/master 9f03110 Jeremy Rubin: Add Basic CheckQueue Benchmark
< bitcoin-git>
bitcoin/master aad4cb5 Jeremy Rubin: Address ryanofsky feedback on CCheckQueue benchmarks. Eliminated magic numbers, fixed scoping of vectors (and memory movement component of benchmark).
< bitcoin-git>
bitcoin/master 054d664 Wladimir J. van der Laan: Merge #9498: Basic CCheckQueue Benchmarks...
< sipa_>
all we need is that the wallet has its own idea of what the best chain is, right?
< sipa_>
so its responses are consistent
< BlueMatt>
morcos: is writing up an issue with two other concerns we just found
< BlueMatt>
even #9570 is a big chunk of code for 0.14
< gribble>
https://github.com/bitcoin/bitcoin/issues/9570 | Block Wallet RPCs until wallet is synced to our current chain by TheBlueMatt · Pull Request #9570 · bitcoin/bitcoin · GitHub
< BlueMatt>
and it would need a few more changes
< sipa_>
sigh
< BlueMatt>
sipa_: did you look at 9570?
< BlueMatt>
its nontrivial
< sipa_>
but is it needed once the wallet has its own idea about the chaintip?
< BlueMatt>
9570 gives the wallet its own idea about the chaintip
< sipa_>
oh
< BlueMatt>
though not in a very full-featured way
< BlueMatt>
but, its a bit too late to be making major changes like that for 0.14, I think
< BlueMatt>
i think at the start of the 0.15 release cycle we should move the wallet callbacks into a separate thread with all these fixes and let it simmer for 0.15
< sipa_>
ok
< BlueMatt>
same with multi-threaded message handler
< BlueMatt>
'cause a lot of these wallet issues on master are only realistic if you call submitblock (though some are also triggerable as a result of the additional ActivateBestChains added in #9375)
< MarcoFalke>
I remember someone was worried about NULL pointer derefs showing up in the release notes and they cause panic, when in fact there should be no reason to panic...
< MarcoFalke>
Should I change the title before merge?
< wumpus>
yes, if you change the title preferably do it before merge
< CodeShark>
luke-jr: license?
< luke-jr>
CodeShark: yes, for the BIP text
< CodeShark>
public domain, not sure what you mean by license
< CodeShark>
Example?
< luke-jr>
ok, PD is acceptable since it predates BIP 2 I guess
< gribble>
https://github.com/bitcoin/bitcoin/issues/9586 | bip68-sequence.py failing on master after recent net changes, due to mocktime interaction · Issue #9586 · bitcoin/bitcoin · GitHub
< sipa>
PLOINK
< jonasschnelli>
\o/
< BlueMatt>
mtg time
< wumpus>
#startmeeting
< lightningbot>
Meeting started Thu Jan 19 19:00:10 2017 UTC. The chair is wumpus. Information about MeetBot at http://wiki.debian.org/MeetBot.
< BlueMatt>
I'd consider 9526 is a bugfix, but i guess i dont care strongly either way
< luke-jr>
#8775 specifically
< gribble>
https://github.com/bitcoin/bitcoin/issues/8775 | RPC refactoring: Access wallet using new GetWalletForJSONRPCRequest by luke-jr · Pull Request #8775 · bitcoin/bitcoin · GitHub
< sipa>
i think 9526 is a bugfix
< luke-jr>
but it seems it conflicted again, so I guess less than ready anyway :x
< BlueMatt>
ok, so to conclude, #9461 and #9294 - 9461 i think is ready-ish (one more look-over, please, its easy?), and 9294 I think we should merge with a few commitments to postumous reviews
< gribble>
https://github.com/bitcoin/bitcoin/issues/9461 | [Qt] Improve progress display during headers-sync and peer-finding by jonasschnelli · Pull Request #9461 · bitcoin/bitcoin · GitHub
< jtimon>
I generally dislike that we fork the branch knowing that some fix will be needed in both branches in advance
< BlueMatt>
9377 we agreed previously was bugfix, and 9526, if we merge it for 14, i'd call a bugfix
< MarcoFalke>
jtimon: There won't be a branch today
< BlueMatt>
jtimon: no, we branch in 2 weeks
< MarcoFalke>
We still have next week to fix bugs
< jtimon>
the whole "we can merge it after fork, because it's a bugfix" concept
< sipa>
the fork is only in 2 weeks
< wumpus>
who is talking about a fork?
< sipa>
bugfixes can go in in between
< wumpus>
bugfixes can be merged, by definition, after the feature freeze
< jtimon>
oh, I see, just mean 0.14 git fork, ie just branching
< wumpus>
because it's a feature freeze nto a bug fix freeze
< jonasschnelli>
Yes. And technically 9294 is kind-of-a-fix for the missed HD chain split in 0.13. And there are no things to fix... only stuff to improve
< sipa>
jtimon: today (or whenever we decide) is the feature freeze. the actual 0.14 branch is only created in 2 weeks
< BlueMatt>
9294 has string changes, so must be today or not at all
< wumpus>
the branch is created at rc1 time
< jtimon>
sipa: thanks I mixed feature freeze with branching
< jonasschnelli>
BlueMatt: Yes. I'm happy to merge it without the Consensus::Params::nPowTargetTimespan change
< wumpus>
(so that releases happen from a branch, not from master)
< jonasschnelli>
If no objections...
< BlueMatt>
jonasschnelli: open a new pr for that change, and then merge it, I'd say
< MarcoFalke>
jonasschnelli: Agree
< BlueMatt>
(merge the one without the Consensus::Params thing, then open a pr to change it)
< jonasschnelli>
Okay.
< jtimon>
so ideally all the bugfixes we know will be merged before branching, forget about my previous comment then
< morcos>
I apologize for not reviewing 9294, but i feel like i never got up to speed enough with the code in question. I do thik that although it's not critical and isn't already tagged 0.14, #9535 could be merged now and i know cfields wants it too
< bitcoin-git>
bitcoin/master 40ec7c7 Jonas Schnelli: [Qt] Improve progress display during headers-sync and peer-finding
< bitcoin-git>
bitcoin/master b250686 Jonas Schnelli: Merge #9461: [Qt] Improve progress display during headers-sync and peer-finding...
< wumpus>
all the bugfixes we know and can realistically make the release (or are critical enough to delay it) should be merged before rc1, yes, thus before the branch
< gmaxwell>
luke-jr: multiwallet pains me. because darn, such a simple set of changes remaining. we need to get out of this mode where all the intensity is in the week before feature freeze. :P (maybe new major version every month. :P )
< sipa>
i think 9525 is pretty trivial
< bitcoin-git>
[bitcoin] jonasschnelli closed pull request #9461: [Qt] Improve progress display during headers-sync and peer-finding (master...2017/01/qt_sync) https://github.com/bitcoin/bitcoin/pull/9461
< sipa>
eh, 9535
< wumpus>
all the intensity isn't in the week before feature freeze, we've merged tons of stuff in the last months
< morcos>
it's got enough ack's are there any objections to 9535 sipa?
< luke-jr>
it's okay
< BlueMatt>
sipa: ok, so press the button? I'd call jtimon's review pretty thourough (even ignoring all the lock testing I plan on doing in the next 2 weeks)
< wumpus>
and some things won't make a release, that's okay
< jtimon>
BlueMatt: I wouldn't call it complete, but I noted the parts I did not do
< wumpus>
priority for 0.14 is solving the nasty remaining issues, like the wallet sync problems
< BlueMatt>
ok, so we need to figure out what to do about #9294, does anyone have any objections to merging so that we can freeze and getting postumous acks?
< morcos>
gmaxwell: please make sure you see this so you don't complain later that you didn't realize we were sticking everything back into cs_main again
< jonasschnelli>
I apologise for 7946,... I wasn't aware that this could cause sync issues
< wumpus>
I tagged 9535 for 0.14 (I uess that's the intent?)
< BlueMatt>
jonasschnelli: ehh, nbd, thats why it was early in a release cycle...sadly no one fixed it before now :(
< morcos>
wumpus: yes or just merge. i think its ready, but not sure why it hasn't been
< BlueMatt>
turns out there is complicated machinery to fix it, eg #9570
< gribble>
https://github.com/bitcoin/bitcoin/issues/9570 | Block Wallet RPCs until wallet is synced to our current chain by TheBlueMatt · Pull Request #9570 · bitcoin/bitcoin · GitHub
< BlueMatt>
but like x2
< BlueMatt>
I'm working on a version of it, but I really dont like that much change this late in cycle
< BlueMatt>
so hopefully we can get the changes in super early in 0.15, and get lots of eyes on it through that cylc
< BlueMatt>
e
< BlueMatt>
^ this is my recommendation
< wumpus>
morcos: well it's not tagged for 0.14, so it has been hidden for me as that's what I've been focusing on
< BlueMatt>
which is merge 9583
< BlueMatt>
wumpus: the issue to track this (9148) has been tagged for 14 all along
< CodeShark>
What's the target date for 0.15?
< cfields>
BlueMatt: which is your recommendation? 9538?
< morcos>
Can anyone think of any downside for merging 9583? Is there any chance we made further changes later that somehow were depending on the fact that we weren't holding cs_main through the wallet updates any more?
< cfields>
BlueMatt: sadly, I think I agree. I've been down the rabbit hole today trying to come up with something simple, and it gets more complicated (and I become less comfortable) quickly.
< morcos>
I can't think of anything htat could make sense, but really thats the only downside I could imagine... Otherwise its just not making an improvement that would have been nice to make..
< CodeShark>
wumpus: thx
< jonasschnelli>
morcos: Yes. Downside is slighly slower sync/rescan
< BlueMatt>
(and I do not believe it is (yet) a major performance regression because this is pretty much all called from the single ProcessMessages thread)
< gmaxwell>
I don't think any design depended on not holding it, varrious testing might have.
< gmaxwell>
jonasschnelli: I don't see how it could result in a slower sync, it's all in one thread.
< gmaxwell>
(and the networking thread doesn't itself grab cs_main)
< cfields>
morcos: isn't there still one site where it gets called without cs_main though?
< jonasschnelli>
Hmm.. I guess I'm wrong. #7946 didn't and it was acctually a stepping stone for stuff that's not PRed.
< BlueMatt>
my intention for 0.15 is to move these callbacks into a background thread asap
< BlueMatt>
cfields: that will not be true after the revert, i think
< morcos>
cfields: Do you mean after the reversion in 9583? I don't think so?
< cfields>
ok, maybe i traced it wrong. Will do again.
< BlueMatt>
ok, if no one has any conceptual objections to 9583, then I dont think there is much to discuss on it now, just note that thoruough review is needed
< wumpus>
any other proposed topics?
< sipa>
sad, but i accept that 9583 is probably the only viable solution for 0.14
< BlueMatt>
indeed
< BlueMatt>
one step forward, one step back, but at least we learned something
< BlueMatt>
2 steps forward for 0.15 :)
< luke-jr>
☺
< jonasschnelli>
We could wrap it in #ifdef WALLET_ENABLED... *duck*
< BlueMatt>
its used in net_processing
< jonasschnelli>
I meant the cs_main lock for SyncTransaction, but just kidding.
< wumpus>
any other proposed topics?
< BlueMatt>
gmaxwell: are we still doing #9501 for 0.14?
< morcos>
wait i'm confused... gmaxwell you don't want those merged before 0.14 or you do?
< jonasschnelli>
I guess he don't..
< morcos>
nm, BlueMatt confused me... importmulti fixes should be in 0.14, i think we all agree
< sipa>
yes
< jonasschnelli>
Ah.. okay. I read it wrong.
< luke-jr>
the impression I got is that gmaxwell just has more work to do on them, and was prioritising stuff before it
< sipa>
agree
< BlueMatt>
I'm ok with untagging #9027 for 14 - it was pointed out that we can do a simple fix to address the issue mentioned there, but there are other issues so its nontrivial to *really* fix
< morcos>
wumpus and sipa when you get a chance, take a look at #9371... i think thats the direction you wanted me to go... and if we do 9583.. its pretty clearly no change in behavior from what txConflicted would have done..
< bitcoin-git>
[bitcoin] paveljanik opened pull request #9587: Do not shadow local variable named `tx`. (master...20170119_Wshadow_net_processing) https://github.com/bitcoin/bitcoin/pull/9587
< wumpus>
morcos: will do
< wumpus>
ok, any other topics?
< sipa>
i propose lunch
< BlueMatt>
I'm done (finally) :p
< BlueMatt>
sipa: too late, already did that
< wumpus>
let's end early then
< wumpus>
#endmeeting
< lightningbot>
Meeting ended Thu Jan 19 19:46:06 2017 UTC. Information about MeetBot at http://wiki.debian.org/MeetBot . (v 0.1.4)
< luke-jr>
sipa: if you get a minute, can you give me at least a text-"verbal" ACK for some copyright license to put on BIPs 30, 32, 62, 66, and 103 please? is BSD-2-Clause okay?
< gribble>
https://github.com/bitcoin/bitcoin/issues/8369 | [FOR LATER USE][WIP][Wallet] add support for a flexible "set of features" by jonasschnelli · Pull Request #8369 · bitcoin/bitcoin · GitHub
< sdaftuar>
sipa: i've been thinking about PrecomputedTransactionData (prompted by jl2012's pr, #9572, where he proposed skipping the calculation for non-segwit tx's)
< sdaftuar>
now that we avoid copying CTransaction's around, by deserializing directly to a shared pointer, which in turn gets stored in the mempool and typically reconstructed into a block via compact block relay, that we could calculate these PrecomputedTransactionData's just once
< sdaftuar>
and store them somewhere, and avoid recalculation in ConnectBlock
< sdaftuar>
my first thought was to just store them in CTransaction itself
< sdaftuar>
which is not so hard to code up, but i don't know that the overhead is worth it in every situation, for instance reading a block off disk to deliver to a peer
< sdaftuar>
or reading a tx off the network that we end up discarding
< sdaftuar>
sipa: anyway i'd be curious to know whether you think this is worth pursuing, and if so what route you'd suggest i try. for instance, i could try adding extra information to CTransaction that may be changed after it's deserialized, but that would undo all the effort you just went through to make it never change after deserialization!
< gmaxwell>
We should generally figure out how to cut out needless computation in reading blocks from disk generally... like we shouldn't be computing hashroots just to reply to a getdata.
< gmaxwell>
(or in wallet rescan)
< sdaftuar>
gmaxwell: yeah that occurred to me as well. i think there are other situations too, though -- such as someone sends you a giant block off the network that you end up not processing (say because it's low work)
< sdaftuar>
we deserialize each transaction and calculate its hash before deciding to ignore it
< sdaftuar>
and my proposed code would have quadrupled the hashing...
< gmaxwell>
I do like the ideal of stapling that stuff to the transaction.
< sdaftuar>
any suggestions on the best way to do it? i've been brainstorming with ryanofsky and bluematt, some of the options that have been proposed include: keeping CTransaction as it is, but adding a new container CHashedTransaction that contains it and adds extra data, and storing that in the mempool
< sdaftuar>
or, adding mutable data to the CTransaction, and possibly also some kind of synchronization primitives so that it can be updated after the fact (? ew)
< gmaxwell>
I was thinking the container thing/
< * gmaxwell>
lunch &
< ryanofsky>
sdaftuar, for the mutable data approach, you could use c++11 call_once (http://en.cppreference.com/w/cpp/thread/call_once) to implement it without having to use low-level synchronization primitives directly
< * luke-jr>
glares at Travis for being so slow
< bitcoin-git>
[bitcoin] morcos opened pull request #9589: Use incrementalRelayFee for BIP 125 (RBF) replacement logic (master...incrementalFee) https://github.com/bitcoin/bitcoin/pull/9589
< bitcoin-git>
[bitcoin] practicalswift opened pull request #9590: Improve readability by removing redundant casts to same type (master...remove-redundant-casts) https://github.com/bitcoin/bitcoin/pull/9590
< achow101>
gmaxwell: when is the alert going out?
< bitcoin-git>
[bitcoin] jnewbery opened pull request #9591: [WIP] count mempool and extra pool matches correctly in PartiallyDownloadedBlock::InitData() (master...compactmatches) https://github.com/bitcoin/bitcoin/pull/9591
< bitcoin-git>
[bitcoin] ryanofsky opened pull request #9592: [Qt] Add checkbox in the GUI to opt-in to RBF when creating a transaction (master...pr/grbf) https://github.com/bitcoin/bitcoin/pull/9592