< wumpus>
jonatack: using @ in your ACK messages is messing with the github-merge script :)
< wumpus>
(it has a detection for those to avoid @... mentions that would cause spam with github's email notification so it makes it warn about @'s in the merge message)
< bitcoin-git>
bitcoin/master 95e61c1 Andrew Chow: Move Hashers to util/hasher.{cpp/h}
< bitcoin-git>
bitcoin/master 210b693 Andrew Chow: Add generic SaltedSipHasher
< bitcoin-git>
bitcoin/master 281fd1a Andrew Chow: Replace KeyIDHasher with SaltedSipHasher
< bitcoin-git>
[bitcoin] laanwj merged pull request #19935: Move SaltedHashers to separate file and add some new ones (master...mv-saltedhash) https://github.com/bitcoin/bitcoin/pull/19935
< wumpus>
darosior: yes, the release notes are in the wiki during the release candidate phase for easy editing
< wumpus>
darosior: invited you to the "frequent contributors" group for bitcoin and bitcoin-core orgs (this is not currently a requirement for editing the wiki but will likely become so)
< fanquake>
wumpus: I like how GH is 2/4 when rendering your avatar in dark mode..
< wumpus>
fanquake: yea it's interesting how many websites don't take transparency in avatars into account and do the weirdest things, github sometimes adds a white circle behind it, sometimes shows it as transparent... and sometimes, adds only weird white lines :)
< bitcoin-git>
bitcoin/master 0d246a5 Anthony Towns: net, net_processing: move NetEventsInterface method docs to net.h
< bitcoin-git>
bitcoin/master 0df3d3f Anthony Towns: net_processing: make more of PeerManager private
< bitcoin-git>
bitcoin/master a568b82 Anthony Towns: net_processing: split PeerManager into interface and implementation classes
< bitcoin-git>
[bitcoin] laanwj merged pull request #20811: refactor: move net_processing implementation details out of header (master...202012-netproc-impl-split) https://github.com/bitcoin/bitcoin/pull/20811
< wumpus>
that said my avatar doesn't really work that well in dark mode (black on grey), should probably make it a color that works in either setting like purple
< bitcoin-git>
[bitcoin] jnewbery opened pull request #20925: RFC: Move Peer and PeerManagerImpl to separate header (master...2021-01-peermanimplh) https://github.com/bitcoin/bitcoin/pull/20925
< norisgOG_>
Hello all I have a question concering signatures in bitcoin, where in the transaction can I see which HASH_Type I have to use to calculate the transaction hash ?
< wumpus>
norisgOG_: #bitcoin please, this channel is for development ony
< wumpus>
vasild: sure, just make a PR that reverts it
< vasild>
ok
< wumpus>
it's too late to rush a revert, but still it's fairly easy to review
< MarcoFalke>
why revert?
< MarcoFalke>
Was it wrong? I think it is just incomplete.
< wumpus>
in that case, better to improve it instead
< wumpus>
the only reason to revert it would be if it's causing major issues to people running master, or there's no clear way forward
< bitcoin-git>
[bitcoin] vasild opened pull request #20926: revert #20852 net: allow CSubNet of non-IP networks (master...revert_20852) https://github.com/bitcoin/bitcoin/pull/20926
< wumpus>
btw: my release notes pull request listing script can't distinguish between gui and main repo PRs, is there a way to do this based on the merge commit message?
< wumpus>
(i had to sort them out manually this time)
< wumpus>
(which is, still, pretty easy "low numbers are gui repo" but not really ideal)
< MarcoFalke>
the merge commits start with "bitcoin-core/gui#123 remove foobar"
< MarcoFalke>
so you can do title.startswith('Merge bitcoin-core/gui#')
< wumpus>
MarcoFalke: thanks
< wumpus>
that sounds easy :)
< wumpus>
I did use the gui#1234 convention in the release notes, likely we need to update the bitcoin-core.org HTML-izing plugin as well to recognize these and generate valid links to them
< vasild>
MarcoFalke: so you decide to ban torv3.onion, do "setban torv3.onion add". The ban is effective and works. 2 days later bitcoin core is restarted. The ban is lost, silently.
< MarcoFalke>
what would happen with the revert?
< MarcoFalke>
does the rpc fail silently or verbosely?
< vasild>
let me see...
< MarcoFalke>
Either way, it shouldn't matter much. We are aware of the issue and know that the file format needs to change. Reverting the pull doesn't help changing the file format.
< vasild>
the log says "SweepBanned: Removed banned node ip/subnet from banlist.dat: ::/0"
< wumpus>
harding: hi! for 0.21.0 we have a new kind of PR which comes from the bitcoin-core/gui repository, I've used the syntax "gui/#xxxx" to refer to these, if we want these to point to the right repository we'll need to update the link conversion
< vasild>
(but nobody reads the log anyway)
< MarcoFalke>
If you are worried that someone is running master and doesn't see the silent fail, you can turn it into a verbose fail. Though I'd rather prefer to spend the resources on fixing the file format
< vasild>
MarcoFalke: ok, but at least if the tries to verify if the ban succeeded he will see it did not. While with the PR it will work for some time, until a restart...
< harding>
wumpus: got it. I think that should, if we do nothing, just not produce a link, which isn't ideal but at least isn't broken. I'll look into making it work. Thanks for the heads-up!
< wumpus>
harding: that's good to know! i was afraid it would make a link to the bitcoin/bitcoin issue of the same number that would be bad, but yes that seems acceptable really
< vasild>
but you are right, the difference is not so big, while the PR still fixes the DisconnectNode() issue properly
< vasild>
What about changing banman to maintain a list of banned IP-subnets (like before 20852) - no format change in banlist.dat. Plus add a list of banned hosts and save it in `banhosts.dat` (always in addrv2 format)
< vasild>
that looks like a perfect forward/backward/sideways file format compatibility
< MarcoFalke>
json probably means we can skip the serialization/deserialization and just dump the rpc argument directly
< MarcoFalke>
Though on startup (when the json doesn't exist) you'd have to convert the banlist.dat once to json. Not sure if that is easily possible.
< vasild>
MarcoFalke: we have CSubNet::ToString()
< MarcoFalke>
oh nice
< vasild>
so, looks like putting the entries in a .json would resolve any file format addrv1 vs addrv2 issues. Will be bigger on disk and slower to write/read, but insignificantly.
< wumpus>
most people aren't going to have that many bans for it to matter wrt start-up performance i guess :)
< MarcoFalke>
The network has 100k nodes, so even if all nodes are banned in the json file, it should be still smaller than the largest wallet.dat we've seen
< wumpus>
i wouldn't suggest doing the same for the peers.dat
< vasild>
100k x ~20 bytes per entry =~ 2MiB
< jonatack>
i used to have a lot of bans, but it's been awhile since i was able to find the banlist, is it still online/maintained?
< vasild>
yes! :-)
< jonatack>
iirc it was 700-1000 entries last time i saw it
< wumpus>
gmaxwell used to maintain a ban list, but don't know if he did for a while
< jonatack>
yes that one
< vasild>
my "yes!" was for the banlist.dat file in the datadir
< vasild>
I have a node that is permanently online and listening on ipv4/6 and banlist.dat is 37 bytes (I guess empty list). Lets focus on whether to put it in settings.json or in banlist.json ...
< wumpus>
a separate file still makes sense imo
< jonatack>
agree, banlist.json
< vasild>
+1
< luke-jr>
is github working poorly for others as well?
< vasild>
luke-jr: poorly == slowly?
< jb55>
like more than normal?
< luke-jr>
vasild: possibly; I clicked "cancel" (to change the scope of my review comment) and it ignored me
< vasild>
luke-jr: I have seen that too - clicks being ignored
< luke-jr>
why is ##bitcoin-core-gui invite-only in the first place?
< wumpus>
to keep me out probably 😀
< luke-jr>
:|
< wumpus>
(TIL that that channel even exists)
< luke-jr>
I'm usually there, but the +i messes up my rejoin on reconnect :/
< achow101>
luke-jr: I think it's because there was a bunch of spammers a few weeks ago
< jb55>
ah yes the antigui folks. ruthless bunch.
< luke-jr>
XD
< luke-jr>
jb55: they only get that bad because Trump incited them <.< *hides*
< wumpus>
usually requiring nickserv sign in (+r) is enough to get rid of spammers
< luke-jr>
plot twist: the "spam" was just my IRC connection dropping/reconnecting in the first place
< prayank>
Hello World. It will be helpful if one of the experienced devs could comment on this issue with : 1. Do we need to fix this? 2. What's the problem? 3. What should be the approach to fix it
< wumpus>
is everyone done editing the release notes for 0.21.0? i'd like to merge them back into the branch
< sipa>
prayank: there are several open PRs that improve coin selection, but there is currently not much activity on them
< sipa>
wumpus: i think that's fine
< wumpus>
if manual coin selection automatically adds an extra input in some case that does definitely sound like a bug
< prayank>
sipa: So the problem is with coin selection algorithm? Why is coin selection being used though if user had selected some inputs in initial tx? Or if it's designed to work like this then a warning is required so that user is aware of it
< sipa>
prayank: eh, the wallet doesn't know that the original transaction was using coin control
< prayank>
Interesting
< sipa>
so i think it's expected that if you use automatic fee bumping, you'll get whatever the coin selection algorithm decides
< sipa>
as for why it's not decreasing the change and instead adding another input... that may be a bug or a deficiency
< luke-jr>
wumpus: ugh, it looks like MarcoFalke simply deleted the Taproot paragraph -.-
< prayank>
sipa: Thanks. I will do more research.
< luke-jr>
wumpus: added it back in, with some rephrasing to hopefully address some of the concerns
< sipa>
luke-jr: nack
< luke-jr>
sipa: nack is not constructive
< sipa>
luke-jr: i've spent enough time arguing to explain my position
< sipa>
i don't see how this is any different from what you had before
< wumpus>
many people disagree on adding that paragraph, including the people who worked on taproot, so we're not going to
< luke-jr>
1) it suggests the bugfix-only releases as alternative upgrade paths
< luke-jr>
2) it clarifies the absense of telemetry in the software in a way that should hopefully not be confusing
< sipa>
but you're still literally arguing for telemetry... just not one built-in to the software
< luke-jr>
sipa: the only "telemetry" already exists
< sipa>
yes, general upgrade patterns matter
< sipa>
upgrading to 0.21.0 does not
< luke-jr>
that's the point of the paragraph - to make it matter
< sipa>
it won't
< luke-jr>
if not, then nothing is lost
< luke-jr>
there are no downsides, just potential upsides
< sipa>
just confusion, imho
< luke-jr>
I rephrased it to avoid confusion
< luke-jr>
if you think you can phrase it better, feel free
< luke-jr>
but this is "perfect is enemy of the good" territory
< sipa>
i did; i added a paragraph about how taproot is included but without activation on mainnet, and that testing can be done on signet
< luke-jr>
that doesn't accomplish the same thing at all
< sipa>
i believe that is the extent to which bitcoin core should be involved with this
< sipa>
it just sounds bizarre, making it sound like bitcoin core's maintainers are going to use 0.21 upgrade numbers to decide how activation happens
< luke-jr>
developers have no business deciding such things in the first place
< sipa>
yes, so why is that paragraph there?
< sipa>
it'
< luke-jr>
because the upgrade speed of the network IS an important factor to making an educated guess on safe activation speed
< sipa>
luke-jr: i agree with that, but i think (a) putting a note in the release notes will not meaningfully affect people's upgrade behavior (b) makes it sound like we (as software maintainers) are taking an active position in its deployment
< luke-jr>
you can ignore the data if you like, but what you're proposing by keeping the paragraph out, is interfering with others freely participating in such data review
< sdaftuar>
luke-jr: that paragraph doesn't add or detract from any data that anyone would review
< luke-jr>
yes it does
< luke-jr>
sipa: I disagree with (a) - if I'm wrong, there's no negative outcome; as for (b), what do you perceive in that way, and how can we avoid giving that impression?
< sipa>
by not talking about activation in our release notes, but just stating the facts: taproot is implemented, not active, can be tested and experimented with... we can add something about how mainnet activation is up for community discussion and will be implemented in a future version when there is agreement on that
< luke-jr>
throwing out the baby with the bathwater is not a solution
< sipa>
you argue that there is a reasonable chance it helps with anything; for that to be the case, either people would (a) adapt their 0.21 upgrade pattern to whatever they'll use for whatever potential release includes activation or (b) adapt their upgrade pattern for the release that includes activation to much what they're doing for 0.21; further it also requires (c) people participating in the
< sipa>
discussion on activation to have confidence...
< sipa>
that either (a) or (b) happens sufficiently take the 0.21 upgrade numbers specifically into account; i believe all 3 are false
< luke-jr>
even if all 3 were false, there is no harm done
< sipa>
yes there is
< luke-jr>
until we try, we won't know if you are right
< sipa>
bitcoin core's developers and maintainers shouldn't be using the release notes to further the goal of activation
< sdaftuar>
^ completely agree
< luke-jr>
not even to the limited extent of helping the community coordinate better data?
< * jeremyrubin>
doubts anyone even reads the release notes
< jeremyrubin>
it's probably better to just do a separate blog post if you have a point to make IMO
< luke-jr>
hmm
< luke-jr>
I mean, if nobody reads them, why do we even write them? >.>
< sdaftuar>
jeremyrubin: i agree with that too. i think using the release notes to advocate one's own viewpoint is inappropriate in the face of disagreement from other contributors (such as the taproot author!)
< luke-jr>
sdaftuar: it's not even a viewpoint
< sdaftuar>
luke-jr: its a viewpoint about messaging
< luke-jr>
I see your point, I agree in that respect
< luke-jr>
hence why I asked
< luke-jr>
[18:56:21] <luke-jr> not even to the limited extent of helping the community coordinate better data?
< jeremyrubin>
I think a blog post is even more effective to that end
< luke-jr>
jeremyrubin: could be. though nothing limits it to one or the other, if we could agree on something
< jeremyrubin>
and w.r.t. "why write them if no one reads them", clearly people "read them". but I think it's unlikely it's anyone who is not already sufficiently plugged in that such a notice would change anything.
< sipa>
i think it's more likely that someone reads this and wonders why it's mentioned at all and what bitcoin core's role in this is, than that it would affect their upgrade behavior
< sdaftuar>
luke-jr: i also wouldn't really want to participate in some kind of shared "help the developers settle whether there's consensus for activation" process, for what it's worth.
< luke-jr>
what do folks think about wumpus simply including something to this effect in the ML release announcement?
< luke-jr>
sdaftuar: doh, I guess it's still too unclear, since it's not about consensus for activation :/
< sdaftuar>
i'm including method, timing, etc as part of all that
< luke-jr>
yeah, not even then - that's all discussion to be had *based on* this (and other) data
< luke-jr>
informed by*
< sipa>
it's a very subtle point, the distinction between consensus for activation, and deciding a safe activation mechanism... i think that distinction exists, but it's probably lost on most people
< sdaftuar>
well the point here is whether there is a group that is being spoken for in gathering information to inform debate, i believe
< sdaftuar>
and i am opposed to creating such a group that is somehow affiliated with the bitcoin core software
< luke-jr>
more of just telling people to speak for themselves
< sdaftuar>
i strongly agree with people speaking for themselves!
< luke-jr>
sipa: too subtle to come up with something that can be understood?
< luke-jr>
sdaftuar: well, that's all this is, to the extent that it's speaking at all
< jeremyrubin>
Has anyone come across an error where calling removed RPC generate *sometimes* returns error code -1, and sometimes the correct -32601?
< jeremyrubin>
The integration test checks that methodnotfound is returned for generate above some version
< jeremyrubin>
but it checks with params passed for generate, so it triggers a diff error!
< jeremyrubin>
I think we should probably return methodnotfound ahead of extraarguments?
< wumpus>
it's a by-effect of using RPCHelpMan in a way that's not entirely how it's supposed to, e.g. generate *implements a RPC call* that always gives an error that the RPC call doesn't exist, that said, I expect it's going to be entirely removed in another version or so, no need to put work in it
< wumpus>
RPCHelpMan needs to do it in this order because the argument check needs to happen before entering the inner function body
< wumpus>
sure, you could rewrite it without using RPCHelpMan, but my point is it really doesn't matter
< jeremyrubin>
it could be fixed by preserving the original args I 'spose
< jeremyrubin>
will fix it thos in the library :)
< wumpus>
why call a non-existing RPC at all :) it's only still there as a service to (human) users following old wiki pages with bitcoin-cli, and who tend to care more about the English message than the return code
< jeremyrubin>
To test that a library handles it correctly :)
< sipa>
do we need a test-only permanently-removed RPC? :p
< jeremyrubin>
the rpc is not removed though
< jeremyrubin>
that's what makes it funny
< jeremyrubin>
because it's an RPC which exists that returns methodnotfound
< sipa>
well, it's an RPC is "removed" state
< sipa>
implementation wise something still exists
< wumpus>
yes it's a bit of an easter egg, it's also intentionally not listed in "help"
< wumpus>
but if you want to deterministically generate a 'method not found' error, send a method name that is unlikely to ever exist like 'sendtoip' :-)
< jeremyrubin>
heh
< sipa>
inflatesupply()
< jeremyrubin>
it's not a huge deal; just a bit of an oddity
< jonatack>
ugh, freenode really does not like my vpn, i keep getting kicked
< luke-jr>
I suppose there's probably some workaround from the commandline or something
< wumpus>
we've held up the release on enough macos bs imo
< midnight>
connectivity to the freenode servers would be the issue due to ping timeout.
< jonatack>
wumpus: yay \,o,/
< midnight>
$5 vps/bouncer would do the same thing for you, and likely safer for you, jonatack
< midnight>
wumpus: also, yay!
< wumpus>
jonatack midnight we did it we can still do releases! (well, this is only the tag... yet 🙂)
< jonatack>
midnight: thanks, yes i should. apparently my vpn is blocked by freenode since years, i should do something else, sorry for the logged in/out noise
< jonatack>
wumpus: yeah, not getting any hopes up prematurely ;p
< achow101>
has this been the longest rc cycle?
< jeremyrubin>
jonatack: it's relevant hah
< sipa>
fjahr, elichai2: i made a quick & dirty port of the secp256k1 safegcd modular inverse code to the muhash3072 field... finalizing is 45x faster with it ;)
< wumpus>
achow101: i'm not sure, 5 rcs is not unheard of at least
< wumpus>
for a major release
< sipa>
not going to PR this until the safegcd inverse code is in libsecp256k1 (and hopefully it's easy to convince the reviewers there to have a look at the muhash3072 version too)
< elichai2>
sipa: damn
< sipa>
elichai2: this isn't because safegcd is so amazing (it is, but not *that* amazing; a dumb extgcd based modular inverse would probably be a lot faster than the ladder approach too)
< elichai2>
It's only 100 loc here, which is impressive
< sipa>
elichai2: 200 loc
< fjahr>
sipa: wow, awesome!
< elichai2>
Oops (1am hehe)
< sipa>
elichai2: peak productivity
< elichai2>
I need to find time to read the long explanation you wrote in the libsecp PR about how the safegcd works, I want to understand how it works.
< sipa>
elichai2: please do; i've tried to keep it very accessible
< fjahr>
will check that out as well to get a better understanding :)