< bitcoin-git> [bitcoin] sanket1729 opened pull request #20100: split policy/error consensus codes for CLEANSTACK, MINAMALIF (master...minimal_if) https://github.com/bitcoin/bitcoin/pull/20100
< bitcoin-git> [bitcoin] achow101 opened pull request #20101: rpc: change no wallet loaded message to be clearer (master...better-no-wallet-error) https://github.com/bitcoin/bitcoin/pull/20101
< achow101> gwillen: ^
< wumpus> the release notes should not be considered documentation, and it's good if the program is self-contained, I'd greatly prefer linking to some actual documentation instead-like the help text of options or other RPC calls
< bitcoin-git> [bitcoin] MarcoFalke pushed 3 commits to master: https://github.com/bitcoin/bitcoin/compare/283a73d7eaea...9dd4de2832e2
< bitcoin-git> bitcoin/master ec3916f Pieter Wuille: Use mockable time everywhere in net_processing
< bitcoin-git> bitcoin/master b6834e3 Pieter Wuille: Avoid 'timing mishap' warnings when mocking
< bitcoin-git> bitcoin/master 9dd4de2 MarcoFalke: Merge #20027: Use mockable time everywhere in net_processing
< bitcoin-git> [bitcoin] MarcoFalke merged pull request #20027: Use mockable time everywhere in net_processing (master...202009_mockable_netproc) https://github.com/bitcoin/bitcoin/pull/20027
< bitcoin-git> [bitcoin] MarcoFalke opened pull request #20103: test: Enable mocktime RPC for all test chains (master...2010-testMockAllChains) https://github.com/bitcoin/bitcoin/pull/20103
< wumpus> but no, there is no precedent for help options that are not RPC, and I'm not sure that's a good idea to introduce, it feels to me a kind of cluttery to use RPC help for that, it does reveal our lack of a more general user documentation. As a workaround isn't there some existing RPC where is would be appropriate?
< wumpus> to be honest I think that error message is OK
< wumpus> it's long but it actually tells something useful
< bitcoin-git> [bitcoin] MarcoFalke pushed 2 commits to master: https://github.com/bitcoin/bitcoin/compare/9dd4de2832e2...b337bd7bc087
< bitcoin-git> bitcoin/master fae7a1c MarcoFalke: fuzz: Configure check for main function
< bitcoin-git> bitcoin/master b337bd7 MarcoFalke: Merge #20065: fuzz: Configure check for main function
< bitcoin-git> [bitcoin] MarcoFalke merged pull request #20065: fuzz: Configure check for main function (master...2010-fuzzMainConfig) https://github.com/bitcoin/bitcoin/pull/20065
< bitcoin-git> [bitcoin] jnewbery opened pull request #20105: [net] Remove CombinerAll (master...2020-10-remove-combiner-all) https://github.com/bitcoin/bitcoin/pull/20105
< hebasto> for those who could be interested in Qt code -- https://github.com/users/hebasto/projects/1
< bitcoin-git> [bitcoin] MarcoFalke opened pull request #20106: cirrus: Use kvm to avoid spurious CI failures in the default virtualization cluster (master...2010-ciOtherVirt) https://github.com/bitcoin/bitcoin/pull/20106
< sdaftuar> rt
< * sdaftuar> notes that could have been worse
< bitcoin-git> [bitcoin] MarcoFalke pushed 2 commits to master: https://github.com/bitcoin/bitcoin/compare/b337bd7bc087...392c6f4fb253
< bitcoin-git> bitcoin/master 907f142 Andrew Chow: rpc: change no wallet loaded message to be clearer
< bitcoin-git> bitcoin/master 392c6f4 MarcoFalke: Merge #20101: rpc: change no wallet loaded message to be clearer
< bitcoin-git> [bitcoin] MarcoFalke merged pull request #20101: rpc: change no wallet loaded message to be clearer (master...better-no-wallet-error) https://github.com/bitcoin/bitcoin/pull/20101
< bitcoin-git> [bitcoin] MarcoFalke opened pull request #20107: doc: Collect release-notes snippets (master...2010-docRelSnip) https://github.com/bitcoin/bitcoin/pull/20107
< dongcarl> vasild, sipa: I might be misremembering, but I recall having a discussion about the versionspace of serializers/unserializers and how we should separate them for different types?
< dongcarl> Oh I see that's still being worked on in #19503
< gribble> https://github.com/bitcoin/bitcoin/issues/19503 | Add parameter feature to serialization and use it for CAddress by sipa · Pull Request #19503 · bitcoin/bitcoin · GitHub
< vasild> yeah
< sipa> dongcarl: yeah, but i didn't want to hold up torv3 stuff with that
< sipa> so i hacen't pushed for it further
< dongcarl> Makes sense!
< wumpus> #19503 and TorV3 would be an interesting combination, but yes I don't think they should hold up each other
< gribble> https://github.com/bitcoin/bitcoin/issues/19503 | Add parameter feature to serialization and use it for CAddress by sipa · Pull Request #19503 · bitcoin/bitcoin · GitHub
< bitcoin-git> [bitcoin] laanwj pushed 2 commits to master: https://github.com/bitcoin/bitcoin/compare/392c6f4fb253...d9de00b3e067
< bitcoin-git> bitcoin/master 1afcd41 John Newbery: [net] Remove CombinerAll
< bitcoin-git> bitcoin/master d9de00b Wladimir J. van der Laan: Merge #20105: [net] Remove CombinerAll
< bitcoin-git> [bitcoin] laanwj merged pull request #20105: [net] Remove CombinerAll (master...2020-10-remove-combiner-all) https://github.com/bitcoin/bitcoin/pull/20105
< aj> meeting?
< jnewbery> e
< sipa> hi
< jonasschnelli> hi
< wumpus> #startmeeting
< lightningbot> Meeting started Thu Oct 8 19:02:35 2020 UTC. The chair is wumpus. Information about MeetBot at http://wiki.debian.org/MeetBot.
< lightningbot> Useful Commands: #action #agreed #help #info #idea #link #topic.
< jnewbery> hi
< hebasto> hi
< aj> hiiii
< wumpus> #bitcoin-core-dev Meeting: wumpus sipa gmaxwell jonasschnelli morcos luke-jr sdaftuar jtimon cfields petertodd kanzure bluematt instagibbs phantomcircuit codeshark michagogo marcofalke paveljanik NicolasDorier jl2012 achow101 meshcollider jnewbery maaku fanquake promag provoostenator aj Chris_Stewart_5 dongcarl gwillen jamesob ken281221 ryanofsky gleb moneyball kvaciral ariard digi_james
< wumpus> amiti fjahr jeremyrubin lightlike emilengler jonatack hebasto jb55 elichai2
< meshcollider> hi
< jonatack> ciao
< luke-jr> hi
< wumpus> FWIW: the 0.21 feature freeze is in a week, it probably makes more sense to discuss what is tagged for the 0.21 milestone as high priority for review at this point
< sipa> that's a long list
< wumpus> it looks like no other topics have been proposed for this week
< jonasschnelli> How is the merge back of the GUI repository handled? MarcoFalke?
< wumpus> yes, it's also likely outdated
< jonasschnelli> (Regarding freeze)
< wumpus> which makes it good to go over it I suppose
< vasild> hi
< wumpus> and at least two of the issues are simply 'follow release process'
< sipa> i think #19954 is pretty much done
< gribble> https://github.com/bitcoin/bitcoin/issues/19954 | tor: complete the TORv3 implementation by vasild · Pull Request #19954 · bitcoin/bitcoin · GitHub
< wumpus> yes
< hebasto> #18077 and #18710 could be moved to 0.22
< gribble> https://github.com/bitcoin/bitcoin/issues/18077 | net: Add NAT-PMP port forwarding support by hebasto · Pull Request #18077 · bitcoin/bitcoin · GitHub
< gribble> https://github.com/bitcoin/bitcoin/issues/18710 | Add local thread pool to CCheckQueue by hebasto · Pull Request #18710 · bitcoin/bitcoin · GitHub
< wumpus> hebasto: ok, thanks
< aj> #19543 doesn't have an associated PR yet?
< gribble> https://github.com/bitcoin/bitcoin/issues/19543 | Normalize fee units for RPC ("BTC/kB" and "sat/B) · Issue #19543 · bitcoin/bitcoin · GitHub
< nehan> 5
< nehan> (typo)
< jonatack> i've been focusing on #19953 the past day or so as it seems to be the highest priority along with tor v3
< gribble> https://github.com/bitcoin/bitcoin/issues/19953 | Implement BIP 340-342 validation (Schnorr/taproot/tapscript) by sipa · Pull Request #19953 · bitcoin/bitcoin · GitHub
< jonatack> aj: i planned to do #19543 after FF as it's a bugfix
< gribble> https://github.com/bitcoin/bitcoin/issues/19543 | Normalize fee units for RPC ("BTC/kB" and "sat/B) · Issue #19543 · bitcoin/bitcoin · GitHub
< hebasto> two drafts also could be moved?
< sipa> all todos are done for taproot, including the json tests in the qa-assets repo
< wumpus> jonatack: so that one needs 0.21 milestone?
< wumpus> aj: seems like it, but also seems not urgent for 0.21?
< wumpus> aj: oh "This needs to happen before the next major release. Otherwise, it will be a breaking change."
< aj> wumpus: yeah
< wumpus> ping MarcoFalke
< sipa> wumpus: i'd very much like to get taproot in 0.21, as the alternative (i expect) is that we'll want it early in 0.22 anyway, which will just complicate backports
< sipa> (the code, not activation obviously)
< wumpus> hebasto: done
< wumpus> sipa: agree!
< jonatack> +1
< meshcollider> I feel like we should really push for #19077 if possible just to have it in the same version as descriptor wallets like we discussed
< gribble> https://github.com/bitcoin/bitcoin/issues/19077 | wallet: Add sqlite as an alternative wallet database and use it for new descriptor wallets by achow101 · Pull Request #19077 · bitcoin/bitcoin · GitHub
< meshcollider> But it's a big PR
< sipa> i haven't paid attention to the PR itself, but it seems it's been making lots of progress lately, including review
< meshcollider> Yeah it's had a decent amount of review already so it's not infeasible
< sipa> i know wumpus had reservations about having it in 0.21
< wumpus> looks like we're starting to run into issues building bdb 4.8 on some platforms at least: #19411
< gribble> https://github.com/bitcoin/bitcoin/issues/19411 | Unable to build BDB 4.8 on macOS Big Sur beta or Xcode 12.0 · Issue #19411 · bitcoin/bitcoin · GitHub
< wumpus> sipa: yes, it seems fairly risky to introduce a new wallet database format (which is used by default) in something last minute
< sipa> --with-incompatible-bdb lalala
< achow101> wumpus: it's only used by descriptor wallets which we already have marked as "experimental"
< wumpus> achow101: they're not created by default for new users?
< achow101> no
< wumpus> in that case I"m oay with it
< sipa> ah, descriptor wallets are not default for new wallets?
< achow101> not yet
< sipa> that's perhaps the best of both worlds... get descriptor wallets and sqlite in 0.21, but neither is default
< wumpus> if it's opt-in I see no problem
< achow101> if descriptor wallets were the default, a lot of tests would be failing :p
< wumpus> wrt #20005 I'm not convinced we should do anything for it, the particular issue doesn't affect anything in our project and clearly in the longer run it's better left solved upstream
< gribble> https://github.com/bitcoin/bitcoin/issues/20005 | memcmp with constants that contain zero bytes are broken in GCC · Issue #20005 · bitcoin/bitcoin · GitHub
< sipa> agree
< sipa> it's good to continue discussing the options, but i don't think there is urgency
< wumpus> sure, but removing the milestone then
< sipa> we're not using gcc 9 or gcc 10 for releases either, i believe?
< wumpus> no, 8 at most
< hebasto> 7.4 for linux x86_64
< wumpus> yes, correct
< wumpus> otherwise there's agreement what is on the milestone right now?
< sipa> not everything will make it, but sure
< sipa> i'm also hoping for 19988
< sipa> given the amount of review it's had the past days
< jnewbery> I think 19988 is ready. It has three or four recent ACKs now
< meshcollider> #19988
< gribble> https://github.com/bitcoin/bitcoin/issues/19988 | Overhaul transaction request logic by sipa · Pull Request #19988 · bitcoin/bitcoin · GitHub
< sipa> 50% test code :)
< wumpus> added to milestone anyway
< sipa> thanks!
< sipa> unrelated: i've noticed that sometimes clicking "show resolved" on github expands everything, and sometimes not
< sipa> does anyone else have this, or know how it's caused?
< achow101> #16463 could be removed from the milestone I guess
< gribble> https://github.com/bitcoin/bitcoin/issues/16463 | [BIP 174] Implement serialization support for GLOBAL_XPUB field. by achow101 · Pull Request #16463 · bitcoin/bitcoin · GitHub
< wumpus> ok done
< wumpus> any other topics for today?
< jnewbery> I have a question about c++17
< aj> wumpus: you pinged about moving https://github.com/users/ajtowns/projects/1 under bitcoin/ ; happy to, might be nice to move the automation into drahtbot at the same time
< jnewbery> we're planning to move to c++17 (remove c++11 compat) in v0.21: https://github.com/bitcoin/bitcoin/issues/16684
< wumpus> no, in 0.22 right?
< wumpus> 0.21 introduces c++17 compilation by default
< jnewbery> oops sorry, yes v0.22
< wumpus> 0.22 is c++17 only
< wumpus> okay
< jnewbery> so after v0.21 branch we're theoretically free to use c++17 features throughout the codebase
< wumpus> yes
< jnewbery> My question was whether it makes sense to try to keep c++11 compatability for consensus code for longer
< wumpus> for new code definitely, as for c++11, please don't file PRs to convert all old code at once
< wumpus> for the sake of doing so
< wumpus> there's no *need* for any compatibility with c++11 anymore
< jnewbery> ie use c++17 features in net processing/wallet/etc, but try to keep consensus code on c++11 for longer
< jnewbery> given that apparently compilers have bugs it might make sense to be more conservative with upgrading to the latest features in consensus
< wumpus> I think that will make things really complicated, even defining what is 'consensus code' is pretty hard right now
< jnewbery> but it's just a half-baked thought
< jnewbery> wumpus: right. It'd be nice if that separation were clearer
< wumpus> or do you only mean 'what is part of libconsensus'
< wumpus> dongcarl's libbitcoinkernel is a nice idea but it's only an idea right now
< aj> libbitcoinkernel? no hits on google
< wumpus> yes, it'd be nice but definitely not around the corner, I don't think it's in any shape to make C++ version depend on it
< wumpus> agree with regard to bugs though
< sipa> almost all of C++17 is in GCC 7, and all of it is in GCC 8
< wumpus> everything is broken let' go shopping
< sipa> so i'd agree as far as being conservative w.r.t. C++ features in consensus-critical code, but i don't think that's necessarily a new idea
< sipa> we always want to be conservative there
< sipa> i don't think we need a "this subset of the code must remain c++11 compatible" rule
< sipa> whether enforced or not
< wumpus> I really thin kwe should make one decision for the entire codebase
< sipa> agree
< wumpus> if the recent discovery means that new versions of C++ are unsafe, that means, let's just give up on C++17 for now
< sipa> i don't think so
< wumpus> I don't particularly want the P2P code to be unsafe either
< sipa> it's a bug in a C89 feature FFS
< jnewbery> sipa: does it make sense to codify what you mean by 'conservative w.r.t c++ features in consensus-critical code'?
< wumpus> we don't have suffiient separateion of consensus-critical code from anything
< wumpus> we need to have that, sure
< wumpus> but it's more than you'd expect if you include util and compat and other indirect dependencies
< sipa> jnewbery: yeah, i'm trying to think what that would mean
< sipa> it's just a thought
< wumpus> honestly I think this is completely seperate from the c++17 question
< sipa> yes, agree
< wumpus> yes, it would be good to isolate the consensus code
< wumpus> then again this was a project since, 2012 or so...
< wumpus> also: leveldb is part of consensus
< sipa> i think the question for C++17 or not just depends on whether we expect build environments that people will want to use for 0.22 support it sufficiently or not
< wumpus> it's not like you can build the consensus code separately anyway
< sipa> indeed
< sipa> we *could* say that libconsensus needs to remain C++11 buildable - but i don't think there is much of a reason for that
< wumpus> I don't think so either
< wumpus> c++17 is already three years old anyway and most c++ compilers implemented it, or features from it, before that
< wumpus> it's not that we're super fast in adopting new c++ standards
< sipa> so let's discuss this after 0.21 branch off, whether we think requiring a C++17 build environment will be a problem by the time 0.22 gets released
< sipa> indeed
< jnewbery> sipa: +1
< jonatack> to summarize, if I may: before feature freeze in one week, please review 19953 (BIPs 340-342), 19954 (tor v3), 19988 (tx relay logic), and 19077 (sqlite wallet)
< wumpus> seems deviating from our plan here on last minute does need a very good reason though
< jnewbery> I am interested in how we judge 'conservative w.r.t c++ features', but not necessarily now
< sipa> wumpus: yeah
< wumpus> I think we need to be conservative in making changes to the consensus code, not so much specifically regarding c++ features
< aj> jnewbery: i think that was just a subset of "conservative in general"
< wumpus> right
< wumpus> which was also my first reply, don't change code for c++17 for the sake of using c++17
< sipa> and perhaps avoid features that reviewers may not be very familiar with - which is correlated but not the same as recently-introduced language features
< wumpus> any other topics?
< sipa> jonatack: +1
< wumpus> honestly I'd feel a lot better if we had focused on isolating the consensus-critical code a long time ago, seems like something people like to talk about but it never really panned out yet
< sipa> wumpus: it's... hard
< wumpus> sipa: yes :/
< wumpus> jonatack: agree!
< jnewbery> wumpus: we're moving in that direction ... slowly. #20049 and #20050 are next
< wumpus> sipa: it's hard but maybe one of the things remaining that's really worth doing
< gribble> https://github.com/bitcoin/bitcoin/issues/20049 | De-globalizing ChainstateManager · Issue #20049 · bitcoin/bitcoin · GitHub
< gribble> https://github.com/bitcoin/bitcoin/issues/20050 | validation: Prune (in)direct g_chainman usage related to ::LookupBlockIndex (bundle 1) by dongcarl · Pull Request #20050 · bitcoin/bitcoin · GitHub
< wumpus> jnewbery: I guess the main problem is that isolating the consensus code means changes to the consensus code which is a risk in itself so hard to do
< wumpus> jnewbery: but good to know!
< aj> wumpus: also that we want to switch between non-consensus and consensus code efficiently in lots of places
< sipa> yes, and given previous attempts at refactoring out consensus code ended us more than once is a worse half-baked state, it's harder to get reviewer enthousiasm for such changes
< wumpus> aj: yes defining an interface that makes sens in itself but doesn't make things a lot slower is another thing
< dongcarl> Definitely non-zero risks, which is why the focus should be on doing it incrementally, and testing it continuously :-)
< wumpus> definitely more interestedi n it than discussions about the icon color though :-)
< dongcarl> Hehe
< sipa> haha
< jonatack> hehe
< sipa> it is as BIP42 predicted
< sipa> "Prominent among them is the discussion on what to call 1 billion Bitcoin, which symbol color to use for it, and when wallet clients should switch to it by default. "
< wumpus> heh!
< wumpus> #endmeeting
< lightningbot> Meeting ended Thu Oct 8 19:59:20 2020 UTC. Information about MeetBot at http://wiki.debian.org/MeetBot . (v 0.1.4)
< achow101> time to add "prophet" to sipa's resume
< sipa> aj: fwiw, 19988 now has "time going backwards" unit tests
< vasild> CAddress::unser: time=1601829922, services=134218813, addr=
< vasild> CAddress::unser: time=1600339981, services=134217741, addr=akinbo7tlegsnsxn.onion:8333
< vasild> jonatack: wumpus: so...
< vasild> in the network there are some peers with services=unusually high value
< sipa> that's 0x0800000d
< vasild> they get in via p2p gossip into addrman without causing problems
< vasild> sipa: right
< vasild> using addrv1
< vasild> then addrman saves it on disk just fine in V3_BIP155 format using READWRITE(COMPACTSIZE(services_tmp))
< vasild> however, on read back from disk it bricks because ReadCompactSize() contains this at the end:
< vasild> if (nSizeRet > (uint64_t)MAX_SIZE)
< vasild> throw std::ios_base::failure("ReadCompactSize(): size too large");
< vasild> and MAX_SIZE is quite low value 0x02000000
< vasild> I wondered before wtf is that limit but decided it is harmless
< sipa> vasild: ehhhh
< sipa> this means we need to bypass that limit when deserializing addrv2
< vasild> so WriteCompactSize() can write >0x02000000 but ReadCompactSize() cannot read it back :-X
< sipa> vasild: it's a protection for the case where CompactSize format is used to actually encode a size
< sipa> and not just a number
< sipa> add a bool to ReadCompactSize to determine whether the limit is enforced or not?
< sipa> i guess we overlooked that in bip152, where it's also not used as a size, but restricted to pretty small numbers
< sipa> let me know if you need help with serialization framework stuff
< pinheadmz> where can i find the current version of bitcoin core in the source code? wihtout using git
< sipa> clientversion.h
< pinheadmz> (sorry meant for pr-reviews-club) thanks sipa
< vasild> hmm
< sipa> ah, it's in config/bitcoin-config.h now
< sipa> which may be generated by somwthing else still
< sipa> version numbers are aoways a mess, due to attempts to aboid having it in dozens of places
< pinheadmz> yeah thats why i expected there to be one source of truth that gets copioed arond
< sipa> it's in configure.ac i think
< sipa> top of the file
< pinheadmz> aha yes, this is what im looking for gracias
< vasild> sipa: how would we pass a bool parameter to ReadCompactSize() from READWRITE(COMPACTSIZE(services_tmp));
< sipa> vasild: you can add template parameters to the compactsize wrapper
< vasild> inroduce READWRITE(COMPACTSIZE_UNBOUNDED(services_tmp))
< vasild> ?
< sipa> i can do that also
< sipa> though i'll be busy the next hour or so
< vasild> I am too sleepy, going to bed now, glad I nailed this down
< vasild> (the problem, not the solution)
< * vasild> bb in 9-10h
< jonatack> vasild: nice! sleepy here too, but it looks like that would explain what we are seeing. back on it tomorrow -- goodnight!
< vasild> jonatack: yeah, your disk is not corrupted! :-)
< sipa> vasild: what timezone are you in?
< vasild> UTC+2, it is 22:57 here
< jonatack> i suspect it's 2300
< vasild> sipa: you?
< sipa> vasild: UTC-7
< vasild> almost on the other side of the world :)
< sipa> lots of october 8th left for me
< jonatack> oh not in nyc then
< sipa> jonatack: not yet
< * vasild> zZzZ
< bitcoin-git> [bitcoin] glozow opened pull request #20109: Release notes and followups from 19339 (master...docs-absurdfee) https://github.com/bitcoin/bitcoin/pull/20109