< roasbeef>
provoostenator: responded elsewhere, but yeh I think the bip should be modified to raise back to 1k to reduce the number of round trips when catching up, the PR on the bip to move from 1k to 100 was merged w/o any comments, iirc bitcoin has settings to limit upload on a per peer basis as well which can kick in, as it's no diff from spamming a perry with getblocks/getdata messages
< bitcoin-git>
[bitcoin] Sjors opened pull request #17246: wallet: avoid knapsack when there's no change (master...2019/10/less-knapsack) https://github.com/bitcoin/bitcoin/pull/17246
< wumpus>
let's try to have some more ACKs on #17165 soon
< bitcoin-git>
bitcoin/master 50037e9 Cory Fields: depends: fix boost mac cross build with clang 9+
< bitcoin-git>
bitcoin/master 366753e Wladimir J. van der Laan: Merge #17231: depends: fix boost mac cross build with clang 9+
< bitcoin-git>
[bitcoin] laanwj merged pull request #17231: depends: fix boost mac cross build with clang 9+ (master...fix-boost-clang9) https://github.com/bitcoin/bitcoin/pull/17231
< wumpus>
what was the flag to make bitcoind log the IPs it's trying to connect to again?
< wumpus>
#17247 is strange, I'm trying to figure out if they're somehow connecting to false peer addresses or that their ISP is resetting bitcoin connections
< bitcoin-git>
bitcoin/master fa92813 MarcoFalke: consensus: Explain why fCheckDuplicateInputs can not be skipped and remove...
< bitcoin-git>
bitcoin/master 90ed98a Wladimir J. van der Laan: Merge #17080: consensus: Explain why fCheckDuplicateInputs can not be skip...
< bitcoin-git>
[bitcoin] laanwj merged pull request #17080: consensus: Explain why fCheckDuplicateInputs can not be skipped and remove it (master...1909-docCheckInputs) https://github.com/bitcoin/bitcoin/pull/17080
< bitcoin-git>
bitcoin/master b05ec41 marcaiaf: Add unit testing for the CompressScript functions
< bitcoin-git>
bitcoin/master 37855ec Wladimir J. van der Laan: Merge #17220: tests: Add unit testing for the CompressScript function
< bitcoin-git>
[bitcoin] laanwj merged pull request #17220: tests: Add unit testing for the CompressScript function (master...add_compress_test_cases) https://github.com/bitcoin/bitcoin/pull/17220
< wumpus>
added 0.19.0 milestone to #17135 and removed it from #17035 (bumped to 0.19.1)
< fanquake>
I will take a look at 17135 this morning
< provoostenator>
wumpus: the Great Firewall is known to reset connections for stuff it doesn't like; it creates the perception of a crappy website.
< wumpus>
fanquake: great! we've been making it less scary
< provoostenator>
But this is the same peer (104) over and over again.
< wumpus>
provoostenator: yes, I've seen this behavior for other things for the Chinese firewall, which is why it worried me
< wumpus>
provoostenator: but I'm no longer so worried, there have been no other reports, something like this would be massive
< provoostenator>
Would be useful if debug=net were to log the IP along with the peer number...
< wumpus>
yes, that's why I asked, I know there's a setting to enable logging of IPs of the peers it connects to, this would hav allowed me to try the peers myself and see if they have the same behavior from here
< wumpus>
e.g. to see if it is a *peer problem* or a network problem
< wumpus>
provoostenator: (104) is not the peer number but the error number
< bitcoin-git>
[bitcoin] MarcoFalke opened pull request #17250: Avoid unused call to GuessVerificationProgress in NotifyHeaderTip (master...1910-NoWrongGuess) https://github.com/bitcoin/bitcoin/pull/17250
< wumpus>
injecting a call to close() seems risky; after that, the fd could be reused for something else (say, a new connection ,or a file), and the program doesn't know
< wumpus>
provoostenator: then again it's only for testing one particular thing, I suppose
< provoostenator>
It's just to test stuff, maybe the Google folks have a better idea:https://github.com/google/tcp_killer/pull/2
< wumpus>
it's just, it can result in nasty corruptions, if say, leveldb decides to open a file next and the P2P layer keeps writing into the fd that's now owned by the file
< wumpus>
but you'll have backups I guess
< provoostenator>
Oh that's not healthy indeed...
< bitcoin-git>
[bitcoin] Sjors opened pull request #17251: net: SocketHandler logs peer id for close and disconnect (master...2019/10/net-socket-peer) https://github.com/bitcoin/bitcoin/pull/17251
< provoostenator>
I put a warning in the PR description
< wumpus>
I don't know a good solution that can work from the process itself
< instagibbs>
sdaftuar_, these slides are actually more complete than I expected, useful resource for starting to understand mempool stuff
< fanquake>
instagibbs is there a public link?
< instagibbs>
he shared on gdrive, with permission I can upload the pdf somewhere?
< bitcoin-git>
[bitcoin] promag opened pull request #17252: 0.19: gui: Make polling in ClientModel asynchronous (0.19...2019-10-backport-17135) https://github.com/bitcoin/bitcoin/pull/17252
< sdaftuar>
instagibbs: please do
< sdaftuar>
if someone wants to turn that into a wiki page, that might be best?
< instagibbs>
I could strip mine the slides, stick it on the wiki
< * luke-jr>
ponders if there's a way to get the ref change date
< sdaftuar>
thanks for that command, but can't seem to find it. i'll spend a couple minutes and see how hard it is to redo (since the point of the cleanup was to make it easy)
< elichai2>
wumpus: so basically I should steer away from anything related to threads?
< wumpus>
elichai2: nah, not specifically: I think the high-level thing is that all the low-hanging boost replacements have been done, and everything that's left has probably been tried to replace over the last few years but failed, be sure to do some research into previous PRs or ask around first
< elichai2>
yeah. I didn't know that was the case. next time i'll search the github before
< wumpus>
e.g. there's probably some annoying death-by-a-thousand-cuts reason why it's still there
< elichai2>
especially with the note in the scheduler
< elichai2>
`// boost::thread / boost::chrono should be ported to std::thread / std::chrono // when we support C++11.`
< elichai2>
so it makes it sound like the only reason is no one cared enough :)
< wumpus>
elichai2: that, or the comment was over-optimistic :)
< wumpus>
I, really really wish we could get rid of the boost::sleep madness and it wasn't like this
< wumpus>
I vaguely remember cfields once did a full replacement of the boost chrono/sleep stuff, including handling thread group interrupts, but even he had to give up !
< instagibbs>
elichai2, I recommend opening an issue with your sad news. Open issues make problems easier to track
< elichai2>
:(
< elichai2>
wumpus: but about the times. isn't it enough to assert now > 0? why was #16117 completley closed?
< wumpus>
elichai2: it probably didn't seem worth it anymore after the bad news
< elichai2>
instagibbs: yeah, I might start with an issue(if doesn't exist yet, I won't make this mistake twice heh) that lists all the known boost right now. and people can comment (and i'll search) for why haven't we replaced them yet. for future references
< wumpus>
opening an issue is probably a good idea, too many people stepped into this trap
< instagibbs>
I'm a fan of opening an issue when closing PRs, if the problem is real, but solution somehow insufficient
< instagibbs>
(if the PR doesn't already link an issue, of course!)
< jamesob>
man, PRs that are like 300+ line changes that aren't scripted diffs considered harmful. having to re-review (and re-review) a giant changeset because review has gone on for another month and a rebase has happened is a drag.
< luke-jr>
typically I do re-review by comparing the diffs
< instagibbs>
300 lines? you're like a baby :)
< * instagibbs>
ducks
< luke-jr>
gdd() { local A="$1"; shift local B="$1"; shift gd "$A" "$@" >/tmp/a gd "$B" "$@" >/tmp/b diff /tmp/{a,b} -u | less }
< luke-jr>
newlines not included
< bitcoin-git>
[bitcoin] fanquake opened pull request #17257: gui: disable font antialiasing for QR image address (master...disable_qr_font_antialiasing) https://github.com/bitcoin/bitcoin/pull/17257
< fanquake>
jamesob 2100 lines waiting for you in 17165
< provoostenator>
Not really. Lots of review work out there, just do it :-)
< BlueMatt>
<suggestor> now that bitcoin core is defaulting to segwit more, I wonder if it would make sense to change the default on spending unconfirmed change to not spend non-segwit unconfirmed change due to malleability.
< instagibbs>
BlueMatt, hmm
< instagibbs>
achow101, you mentioned something about wanting to work on coin selection again, if you want to mention it here? (I don't know what you were thinking)
< provoostenator>
Coin selection already prefers confirmed coins, so that seems a bit overkill.
< instagibbs>
true, it heavily tries deeply-then-shallow-confirmed
< achow101>
instagibbs: yes, planning on doing some coin selection stuff and reviving SRD
< meshcollider>
Thanks to everyone who has been reviewing #16341, it's getting very close now which is awesome :)
< gribble>
https://github.com/bitcoin/bitcoin/issues/16341 | Introduce ScriptPubKeyMan interface and use it for key and script management (aka wallet boxes) by achow101 . Pull Request #16341 . bitcoin/bitcoin . GitHub
< provoostenator>
I went down the coin selection rabbit hole yesterday. Happy to review attemps by others :-)
< instagibbs>
Sorry I haven't had time to re-review SPKM PR :/
< instagibbs>
Honestly knapsack has to die
< achow101>
BlueMatt: would there even be non-segwit unconfirmed change unless the wallet is configured to addresstype=legacy?
< instagibbs>
Previously I had tried working on the wallet again, and threw my hands up in dispair because of the knapsack loop behavior
< achow101>
errr changetype=legacy.. I don't think we make non-segwit change anymore?
< BlueMatt>
achow101: no, you need to avoid spending any change that came from a transaction with non-segwit inputs
< BlueMatt>
cause that underlying tx is malleable
< achow101>
oh right
< instagibbs>
achow101, so personally speaking, I'd strong concept ACK killing off knapsack if the UTXO simulation story doesn't look awful
< instagibbs>
imo it's limped along by fear of removing the current UTXO vacuum(that doesn't benefit small wallets anyways!)
< bitcoin-git>
bitcoin/master 04dbdd6 Sjors Provoost: [net] SocketHandler: log peer id for close and disconnect
< bitcoin-git>
bitcoin/master 25d7e2e fanquake: Merge #17251: net: SocketHandler logs peer id for close and disconnect
< provoostenator>
Yeah that loop is horrible. It doesnt' even really start at the top: splitting the fee amongst outputs is done at the top, based on the last iteration.
< bitcoin-git>
[bitcoin] fanquake merged pull request #17251: net: SocketHandler logs peer id for close and disconnect (master...2019/10/net-socket-peer) https://github.com/bitcoin/bitcoin/pull/17251
< provoostenator>
^ yolo merge, before Travis ready :-)
< instagibbs>
It makes improvements to the wallet basically impossible :(
< instagibbs>
some improvements*
< bitcoin-git>
[bitcoin] adamjonas opened pull request #17258: Fix issue with conflicted mempool tx in listsinceblock (master...listsinceblock-filter-conflicts) https://github.com/bitcoin/bitcoin/pull/17258
< instagibbs>
anyways, stepping off soapbox
< instagibbs>
f.e., having the wallet implicitly CPFP
< achow101>
BlueMatt: I suppose that idea is reasonable. but, on the topic of unconfirmed change, how do we handle fee bumping a tx from which we've spent the unconfirmed change?
< provoostenator>
(or some variant thereof)
< instagibbs>
provoostenator, right, CPFP would have worked for me... only for CPFP case :)
< BlueMatt>
you dont
< BlueMatt>
you bump the next one up
< BlueMatt>
cpfp :)
< instagibbs>
but I felt that wasn't worth it
< jnewbery>
topic request: #16341
< gribble>
https://github.com/bitcoin/bitcoin/issues/16341 | Introduce ScriptPubKeyMan interface and use it for key and script management (aka wallet boxes) by achow101 . Pull Request #16341 . bitcoin/bitcoin . GitHub
< achow101>
BlueMatt: true. I was about to suggest that maybe we shouldn't allow unconfirmed change at all
< achow101>
And with upcoming more complex scripts, maybe we shouldn't, because we aren't necessarily the sole owner of those outputs
< provoostenator>
Instead of spending unconfirmed change, we could RBF and add an output?
< provoostenator>
That would be a nice fee saving UX improvment.
< provoostenator>
(though non trivial if the pre-RBF transactions gets mined)
< instagibbs>
that's the sticky part
< provoostenator>
You can sign two variants and start broadcasting the fallback if needed.
< provoostenator>
But that's less of a set-and-forget than we have now.
< instagibbs>
our wallet is very "transactional" i nthe wrong sense currently
< provoostenator>
set-and-quit-and-forget I mean
< instagibbs>
transaction as in CTransaction rather than logical transaction :)
< achow101>
next topic?
< meshcollider>
#topic SPKM PR #16341 (jnewbery)
< gribble>
https://github.com/bitcoin/bitcoin/issues/16341 | Introduce ScriptPubKeyMan interface and use it for key and script management (aka wallet boxes) by achow101 . Pull Request #16341 . bitcoin/bitcoin . GitHub
< achow101>
ack 'n merge pls
< meshcollider>
Go and bug sipa first, he said he wanted to review it
< jnewbery>
I'd like to help but there's no way I have time to review the PR in the way that it's structured
< provoostenator>
jnewbery: did you also look at ryanofsky's variant, with the same end result?
< achow101>
jnewbery: do you want me to use ryanofsky's structure?
< provoostenator>
I found it useful to review achow101's version first, and then ryanofsky's version
< jnewbery>
provoostenator achow101: I haven't looked at Russ's version yet
< jnewbery>
it's mostly about the volume of changes
< provoostenator>
But I wouldn't either is ideal. Russ' version does a _lot_ of stuff in big increments in the beginning. But his later commits are perhaps better.
< jnewbery>
I know it'd take me at least a week to review so much code and satisfy myself that there aren't bugs
< provoostenator>
It's a huge PR indeed. But it's not obvious how to refactor the giant ball of spaghetti in multiple steps
< ryanofsky>
just catching up but my branch has two big commits at the beginning. one is 100% moveonly. one is a bunch of renames
< ryanofsky>
every other commit is small
< meshcollider>
Yeah this has come up in discussion a few times, noone has come up with an alternative to either this or more massive commits
< jnewbery>
ryanofsky: would only merging those first two commits leave us in a bad state (would it be possible to slice those off in their own PR)?
< jnewbery>
I can definitely review move-only/rename commits
< ryanofsky>
merging those two commits would be fine, as far as i know
< jnewbery>
achow101: what do you think about that approach?
< meshcollider>
If we are going to split this PR up a bit, maybe we should feature-freeze the wallet until all the parts are in to avoid things getting messy half way
< achow101>
jnewbery: I haven't looked at russ's branch in detail
< provoostenator>
Maybe the "Refactor: Split up CWallet and LegacyScriptPubKeyMan and classes" can split into one commit that introduces the class, and one that moves the methods over?
< instagibbs>
that's the other thing true, you're going to force people to rebase 2+ times
< instagibbs>
in serious ways
< meshcollider>
Well they can just wait til it's all in ^
< ryanofsky>
just to clarify my github comment from a few weeks ago, i'm fine if achow's pr is merged as is, i just wouldn't want my ack to be a deciding factor
< ryanofsky>
it's too many scattered changes that i don't fully understand to be confident about all of them
< jnewbery>
That's my main concern (separate from not being able to review it myself):I'm worried that due to the size of this PR, other people who have reviewed it haven't been able to do so in sufficient detail to avoid merging bugs
< achow101>
ryanofsky: I think the only reason acks came in quickly after yours is because they were just waiting for you to be done asking for changes before commenting
< meshcollider>
Yeah it is enormous, I'm feeling the same way
< ryanofsky>
achow101, yes you're right i probably misinterpreted the timing
< meshcollider>
That's why I want to merge it early on in the 0.20 release cycle so we have plenty of time to identify bugs before we get close to a release
< instagibbs>
Yes, I had reviewed once, then waited for velocity to stop, then reviewed the diff between the version I'd reviewed and tip. That said, it deserves more eyes
< meshcollider>
Smaller PRs would definitely encourage more reviews too
< ryanofsky>
meshcollider, i don't think this has to be an all or nothing thing merged early in the release cycle. you could merge the moves early, and then let all the little behavior changes get reviewed as normal
< provoostenator>
For release cycle it doesn't matter much if we wait another week or two for proof-of-newbery
< jnewbery>
I'm raising it because I'm not getting the sense from any of the reviewers who have ACKed that they're super confident
< achow101>
I can try to break it up further based on ryanofsky's branch as it seems like people are okay with some big moveonlys and renames
< jnewbery>
achow101: thanks! I'll also try to take a look at ryanofsky's version next week
< achow101>
if we want to do it in smaller prs, then I think we should feature freeze the wallet
< sipa>
(speaking as someone who has not looked at the changes in detail, but plans to) big moveonlys are still fairly mechanically verifiable, regardless of size
< ryanofsky>
yeah i think the first big commit which is completely moveonly is basically trivial
< achow101>
(now I just need to find ryanofsky's branch, I seem to be clikcing all the wrong "Load more comments..")
< ryanofsky>
the second big commit which is the "rename" commit is kind of eyeglazing but still boring and reviewable i think
< MarcoFalke>
Can this channel be opened again or at least mention in the title that log in is required?
< MarcoFalke>
People keep running into this issue
< meshcollider>
achow101: I'm happy to do that for a while because otherwise this is going to take forever to get in
< instagibbs>
if feature freeze is the plan, I think people should be allowed a window to squeeze in near-merged features :)
< instagibbs>
with heads up
< instagibbs>
unless the freeze is like... a week
< achow101>
I think some large-ish changes could also become scripted-diffs, so that will help review
< ryanofsky>
i'm fine with a feature freeze, but not sure what a feature freeze would be doing...
< jnewbery>
If ryanofsky's branch really is two big commits (that become the first PR) and then a bunch of small commits, I also don't understand the need for a feature freeze
< jnewbery>
it sounds like the disruptive rebasey part would be those first two commits
< sipa>
i think it's simpler to have reviewers lined up for a "it will be merged on day X", and then the PR author can rebase that day exactly, everyone give a final ack, and it's merged
< jnewbery>
and once they're in they're in
< ryanofsky>
yeah exactly all the commits are small and normal except 2
< instagibbs>
ok provided you think it's only 1 painful break, fine
< ryanofsky>
if the concern is difficulty rebasing the 2 big commits, i've done than several times, and it hasn't been a big deal. can continue to do it if desired
< instagibbs>
let's just do that
< meshcollider>
Alright achow101 happy?
< achow101>
ok
< meshcollider>
Sweet, any other topics?
< provoostenator>
achow101: if you're replacing the branch, please make a new PR...
< achow101>
yes, new prs will be opened
< jnewbery>
thanks!
< provoostenator>
Great, so I don't have to write a Chrome plugin to auto-click "Load More..."
< meshcollider>
Please do that anyway :p
< JeremyCrookshank>
Hello?
< instagibbs>
since #16944 got un-scope-creeped, I think it's close to merge (selfish reminder)
< provoostenator>
Yes, it can be merged independent of the keypool stuff; it just won't work for keypool-less watch-only wallets.
< JeremyCrookshank>
How often are the meetings?
< instagibbs>
JeremyCrookshank, wallet meetings i think are every-other Friday, beginning of this hour
< instagibbs>
main meeting is weekly, Thursday, same time
< JeremyCrookshank>
Thank you :)
< jnewbery>
(scheduled in UTC, so it might be a different local time for you next week depending on where you live)
< instagibbs>
right, iceland time ;)
< achow101>
My main concern with using ryanofsky's commits is the second one, it's not just a rename as it does introduce LegacyScriptPubKeyMan and changes some wallet things to use that
< ryanofsky>
yes it could be broken down, sjors has suggested doing rpc file separately for example
< ryanofsky>
but everything there is a mechanical change, renaming, wrapping aliasing, no changes in behavior. it's just big
< ryanofsky>
easiest way to see that is to run the git diff commands in the commit description
< provoostenator>
And maybe also have one commit that introduces the (Legacy)ScriptPubKeyMan class itself
< achow101>
eyeglazing is an understatement for that commit
< achow101>
how many PRs do people want? 2? more?
< ryanofsky>
provoostenator, i wasn't sure what you meant there. the class without the methods would just be an empty class declaration
< ryanofsky>
achow, one awkward thing is those 4 commits before the 2 big ones
< achow101>
ryanofsky: yeah, I was going to see if I could break this up such that those 4 would slide in before whatever changes they are required for
< achow101>
may need 4 PRs for that
< ryanofsky>
looking
< ryanofsky>
i wonder if we could just merge those now
< achow101>
I'm also looking to drop or merge together the "unsure if necessary" commits with other commits
< achow101>
also gonna base on mater
< achow101>
*master
< ryanofsky>
otherwise i can update my branch to just put the two big commits first
< achow101>
ryanofsky: I'm pretty sure they can fit in other places. They originally were placed before the commits that required them but moved because semi-unrelated
< ryanofsky>
which "unsure if necessary" commits do you think are good?
< achow101>
ScriptPubKeyMan unique_ptr and include <functional.h>
< ryanofsky>
ok why the unique pointer one? it just seems to create a dummy wrapper that doesn't do anything
< achow101>
wrong commit, nvm
< achow101>
include functional is the only one to keep
< ryanofsky>
got it, i understand that one now
< provoostenator>
ryanofsky: I meant class with function defitions and maybe noop methods. That way the function bodies can move from one structure to another. But ignore if impractical.
< ryanofsky>
i just moved the two big commits earlier. i think the first three commits would make a good pr: two moveonlys and the eyeglazing rename
< ryanofsky>
compilation and tests should still pass the whole way through but can reconfirm
< JeremyCrookshank>
ping
< ryanofsky>
pong
< sipa>
pang
< za-kk>
peng
< bitcoin-git>
[bitcoin] achow101 closed pull request #16341: Introduce ScriptPubKeyMan interface and use it for key and script management (aka wallet boxes) (master...box-the-wallet) https://github.com/bitcoin/bitcoin/pull/16341
< bitcoin-git>
[bitcoin] achow101 opened pull request #17260: Split some CWallet functions into new LegacyScriptPubKeyMan (master...wallet-box-pr-1) https://github.com/bitcoin/bitcoin/pull/17260