< sipa>
dongcarl: not just because nobody has gotten around to fix them; PRs which only change style are not permitted
< sipa>
more because that code hasn't been rewritten since
< dongcarl>
sipa: good to know! So usually style changes are packaged with actual changes?
< sipa>
yes
< sipa>
dongcarl: it's in the developer notes :)
< sipa>
Various coding styles have been used during the history of the codebase, and the result is not very consistent. However, we're now trying to converge to a single style, which is specified below. When writing patches, favor the new style over attempting to mimic the surrounding style, except for move-only commits.
< sipa>
Do not submit patches solely to modify the style of existing code.
< dongcarl>
Haha k I might bundle in some style changes for the files I’ve touched for Banman
< sipa>
don't "bundle"
< sipa>
just use the new style when writing new code
< dongcarl>
Right, okay, in that case what I have now should be sufficient
< sipa>
dongcarl: first line in your patch already violates it :p
< sipa>
seems that's also almost the only one
< * dongcarl>
is ashamed
< sipa>
:p
< bitcoin-git>
[bitcoin] sipa opened pull request #14626: Select orphan transaction uniformly for eviction (master...201810_uniform_orphan_eviction) https://github.com/bitcoin/bitcoin/pull/14626
< warren>
sipa: "<sipa> warren: BIP150/BIP151 just need a bit of entropy at connect time" ... but connect time is an event triggered by arbitrary network connections.
< sipa>
warren: accepting a connection already requires orders of magnitude more work than generating a random number, even with a ridiculously slow RNG
< sipa>
warren: stop freaking out; all i meant to say was that our RNG isn't fast compared to what is possible on state of the art, but it's more than enough for what we need, even taking things like encrypted connections into account
< warren>
OK.
< bitcoin-git>
[bitcoin] merland closed pull request #14553: [wip] qt: Fix wrong unit on hourly progress increase (master...progress-increase-per-h) https://github.com/bitcoin/bitcoin/pull/14553
< bitcoin-git>
[bitcoin] murrayn opened pull request #14628: Trivial: Rename misleading 'defaultPort' to 'rpc_port' (master...rpc_port) https://github.com/bitcoin/bitcoin/pull/14628
< wumpus>
phantomcircuit: looking at it
< wumpus>
can we get some reviews for #14532? at one point everyone seemed concerned about this issue, and not it just lingers
< gribble>
https://github.com/bitcoin/bitcoin/issues/14532 | Never bind INADDR_ANY by default, and warn when doing so explicitly by luke-jr · Pull Request #14532 · bitcoin/bitcoin · GitHub
< bitcoin-git>
bitcoin/master f8c1714 Andrew Chow: Convert non-witness UTXOs to witness if witness sig created...
< bitcoin-git>
bitcoin/master 862d159 Pieter Wuille: Add test for conversion from non-witness to witness UTXO
< bitcoin-git>
bitcoin/master f6df989 Wladimir J. van der Laan: Merge #14197: [psbt] Convert non-witness UTXOs to witness if witness sig created...
< bitcoin-git>
[bitcoin] laanwj closed pull request #14197: [psbt] Convert non-witness UTXOs to witness if witness sig created (master...psbt-utxos) https://github.com/bitcoin/bitcoin/pull/14197
< bitcoin-git>
[bitcoin] jnewbery opened pull request #14631: [tests] Move deterministic address import to setup_nodes (master...deprecate_generate2) https://github.com/bitcoin/bitcoin/pull/14631
< achow101>
wumpus: while you're at it, merge #14377?
< gribble>
https://github.com/bitcoin/bitcoin/issues/14377 | check that a separator is found for psbt inputs, outputs, and global map by achow101 · Pull Request #14377 · bitcoin/bitcoin · GitHub
< sipa>
MarcoFalke: idea for the conflict checker... have DrahtBot post a dummy comment on every PR as soon as it's opened, and then update that comment whenever needed
< wumpus>
that... sounds like a good idea
< wumpus>
achow101: ok
< wumpus>
could keep updating the same post instead of posting new ones would save on some mail
< luke-jr>
sipa: that will break the email notifications :/
< luke-jr>
(updating it sounds fine though)
< sipa>
luke-jr: i don't think the email notifications are useful (they mostly mess up my workflow in trying to find recently updated PRs to review... and then realize that they haven't been touched in 2 months, but someone just opened a tiny refactor that conflicts with it)
< sipa>
the "needs rebase" messages are kinda useful, i think
< bitcoin-git>
bitcoin/master 4fb3388 Andrew Chow: check that a separator is found for psbt inputs, outputs, and global map
< bitcoin-git>
bitcoin/master 51e5ef3 Wladimir J. van der Laan: Merge #14377: check that a separator is found for psbt inputs, outputs, and global map...
< bitcoin-git>
[bitcoin] laanwj closed pull request #14377: check that a separator is found for psbt inputs, outputs, and global map (master...fix-psbt-seps) https://github.com/bitcoin/bitcoin/pull/14377
< luke-jr>
sipa: I think the initial notification after opening might be useful to get the content emailed, I mean
< luke-jr>
ie, just skip the dummy post
< wumpus>
I don't think the email notifications for the bot are useful either
< wumpus>
sure, the 'needs rebase' mail is useful for the author of the PR but not others subscribing to it
< wumpus>
I think that's the crux, most of the information is aimed at the author but everyone somehow involved will get the mail, I get so much github notifications I can't pay much attention to them
< wumpus>
(though I have notifications specifically tagging me sorted differently)
< sipa>
i don't generally use the notifications at all; but at least "sort by recently updated" was useful before the bot :)
< luke-jr>
hmm
< luke-jr>
maybe the bot should hook in like Travis
< gmaxwell>
breaking sort by recently updated has probably reduced the amount of review I do by 80%, FWIW.
< gmaxwell>
as I'd typical go and sort by updated and check in on all the active PRs.
< wumpus>
luke-jr: yes that would be preferable, but I do not know if github provides that functionality to random developers, lacking that, sipa's idea to post once then update is a good idea
< sipa>
actually, a top comment that gets updated may not be enough to un-break it; the PRs that get newly referenced by another post's "conflicts with" will still be marked as recently updated
< gmaxwell>
another alternative would be to just post some external thing with the real recently updated data.
< gmaxwell>
e.g. go scrap github or new commits every couple hours and post a report somewhere.
< gmaxwell>
scrape*
< wumpus>
gmaxwell: ah yes kind of what bitcoinacks.com does
< gmaxwell>
(I don't recall that but I figure I wouldn't have noticed because it would have been infrequent)
< sipa>
MarcoFalke: i think that would be useful if it's not too much work
< MarcoFalke>
Should be ~zero work. Static html should be sufficient
< sipa>
this is obviously some strange usage of the word zero that i wasn't previously aware of :)
< MarcoFalke>
~zero == about zero == little
< MarcoFalke>
ryanofsky actually brought up the idea to have one comment per pull request that gets updated. That would be a bit more work, depending on how much I want to solve edit conflicts. (The bot runs in multiple processes)
< MarcoFalke>
Also it'd mean more empty comments
< sipa>
just one per PR
< ryanofsky>
i think it'd be nice if draftbot just claimed and kept updating the first comment in every pr. especially if it could tell you useful status information, like how many acks the pr has
< sipa>
that would be great
< sipa>
(but quite a bit of work i imagine)
< sipa>
some review on #13501 would be welcome; it may fix some of the appveyor spurious failures
< jnewbery>
MarcoFalke: sort-by-most-recent-change used to be the only thing I used, but it's mostly not very helpful now
< bitcoin-git>
[bitcoin] kostyantyn opened pull request #14633: Fix height serialization inside script of coinbase input (master...fix_height_serialization_in_coinbase) https://github.com/bitcoin/bitcoin/pull/14633
< pierre_rochard>
ryanofsky I added a question on the GH issue, for potential or current users of bitcoinacks: is "most-recent-change" preference to be the PR comment thread (excluding bots) or to the actual PR code?
< pierre_rochard>
I can do both, I just want to avoid making the table wider than it already is if one is clearly more useful for reviewers
< bitcoin-git>
[bitcoin] JBaczuk closed pull request #14610: Docs: correction to test readme compile instructions (master...fix_test_readme_compile_instructions) https://github.com/bitcoin/bitcoin/pull/14610
< achow101>
we should make a gribble command for the meeting ping
< sipa>
on the list are #14532 #14350 #14046
< gribble>
https://github.com/bitcoin/bitcoin/issues/14532 | Never bind INADDR_ANY by default, and warn when doing so explicitly by luke-jr · Pull Request #14532 · bitcoin/bitcoin · GitHub
< sipa>
we do not care about the ability to create such wallets anymore, but as long as they're supported we should test them - especially we should test upgrade scenarios
< jnewbery>
ah, thanks Marco. I'll take a look at that
< luke-jr>
hmm
< luke-jr>
so we might actually want to run the wallet tests against N different wallets
< sipa>
i guess we may want to discuss different approaches on #14536?
< sipa>
then some preparation work for being able to use descriptors instead of keypools (which requires logic for caching pubkeys etc), i plan to work on that
< meshcollider>
Yep I'll rebase it as soon as 14565 is in
< provoostenator>
Sweet, that should make achow101's hardware wallet stuff easier too.
< provoostenator>
Feel free to tag me on those PRs, in case I miss them.
< sipa>
with follow up some refactoring to move the existing keypool/ismine logic behind an abstraction that can be instantiated with the old logic, or descriptors (so we can natively import descriptors)
< achow101>
provoostenator: instagibbs: the plan is to make hwi do things with descriptors instead of the different pubkey stuff I was doing earlier
< sipa>
and i think independently there is also a possibility for a few more RPCs now, like PSBT signing that takes descriptors as inpit
< instagibbs>
achow101, That was my assumption
< achow101>
so i'll have to rebase #14075 on top of 14491
< gribble>
https://github.com/bitcoin/bitcoin/issues/14075 | Import watch only pubkeys to the keypool if private keys are disabled by achow101 · Pull Request #14075 · bitcoin/bitcoin · GitHub
< achow101>
probably
< instagibbs>
should be simply
< sipa>
there are other wallet related topics, like ryanofsky's wallet separation
< sipa>
ryanofsky: here?
< ryanofsky>
yes sir
< sipa>
or maybe we can bring that up in tomorrow's meeting
< provoostenator>
Thanks for the quick overview, happy to wait until tomorrow for more details.
< sipa>
it's on high priority too, so hopefully it can get some more attention
< sipa>
if that's enough for this topic, i have another one myself: appveyor failures
< phantomcircuit>
sipa, the wallet stuff seems to be sort of a specialized thing
< phantomcircuit>
it's pretty difficult to maintain any idea of how it's working unless you're spending a lot of time looking at it
< provoostenator>
The refactoring seems to be taking it to a place where it's easier to understand.
< sipa>
phantomcircuit: yeah... i hope it will improve in the future
< provoostenator>
I generally find the "after" code a lot more readable than the "before" code.
< provoostenator>
And descriptors are very useful.
< luke-jr>
they even do the dishes
< sipa>
ha
< sipa>
#topic appveyor failures
< sipa>
i find it pretty annoying that appveyor currently spuriously fails quite frequently
< sipa>
provoostenator: it's doing MSVC builds and MinGW on windows, which aren't used for any production binaries
< sipa>
so they're useful in likely testing for other types of issues by means of platform variety, but they're not necessarily issues that affect real production deployments
< luke-jr>
sipa: well, we don't test Windows binaries when built on gitian/Linux, right?
< luke-jr>
with CI I mean
< sipa>
luke-jr: that's fair!
< sipa>
anyway, i'd just very much like to get some attention to improving this, as continuously seeing red crosses in travis is pretty annoying
< luke-jr>
for now I just ignore the appveyor failures on my PRs
< sipa>
anyone have other topics?
< meshcollider>
It is quite easy to restart appveyor in the same way to restart Travis btw, if it fails
< sipa>
meshcollider: yes, i've restarted dozens of appveyor failures the past days...
< sipa>
#topic open floor: what are people working on?
< jarthur>
I'd bring up unix socket RPC topic again, but haven't had time to think about it. It sounded like folks are ok if it needs to go forward on bitcoind even if cli only has TCP, provided there are thorough tests.
< luke-jr>
rebasing all my PRs :x
< sipa>
jarthur: i personally am ok with that, especially since we have an option to use a different HTTP implementation in bitcoin-cli
< achow101>
psbt + hww stuff ..... and taking lots of exams
< jnewbery>
sipa: still not enough, but hoping to spend much more time reviewing wallet PRs before the end of the year
< sipa>
achow101: good luck!
< sipa>
i've picked up looking at private authentication again (being able to tell a peer is one of multiple acceptable peers who pubkey you know, but they don't learn who you were looking for, and you don't learn who they are, just that they're part of your acceptable set)... this is probably too novel to deploy, but it's a fun exercise
< sipa>
jnewbery: cool :)
< instagibbs>
jnewbery, same, as well as HWI assistance
< meshcollider>
I want to continue focusing on the wallet rework and everything mainly and reviewing wallet stuff like jnewbery
< meshcollider>
And exams like achow101 xD
< sipa>
perhaps you guys should collaborate on the exam thing? :p
< sipa>
ok, any more topics?
< meshcollider>
sipa: lol, he can write it and I'll review :)
< luke-jr>
LOL
< luke-jr>
open source exam answers
< sipa>
#endmeeting
< lightningbot>
Meeting ended Thu Nov 1 19:48:24 2018 UTC. Information about MeetBot at http://wiki.debian.org/MeetBot . (v 0.1.4)
< bitcoin-git>
[bitcoin] ryanofsky opened pull request #14636: Avoid using numeric_limits for sequence numbers and lock times (master...pr/climit) https://github.com/bitcoin/bitcoin/pull/14636
< meshcollider>
maybe appveyor could call drahtbot in a build_failure webhook and then report success anyway
< meshcollider>
then drahtbot could just comment or something