19:00:12 <wumpus> #startmeeting 19:00:12 <lightningbot> Meeting started Thu Nov 5 19:00:12 2020 UTC. The chair is wumpus. Information about MeetBot at http://wiki.debian.org/MeetBot. 19:00:12 <lightningbot> Useful Commands: #action #agreed #help #info #idea #link #topic. 19:00:16 <achow101> hi 19:00:24 <hebasto> hi 19:00:26 <wumpus> #bitcoin -core-dev Meeting: achow101 aj amiti ariard bluematt cfields Chris_Stewart_5 digi_james dongcarl elichai2 emilengler fanquake fjahr gleb gmaxwell gwillen hebasto instagibbs jamesob jb55 jeremyrubin jl2012 jnewbery jonasschnelli jonatack jtimon kallewoof kanzure kvaciral lightlike luke-jr maaku marcofalke meshcollider michagogo moneyball morcos nehan NicolasDorier paveljanik 19:00:27 <wumpus> petertodd phantomcircuit promag provoostenator ryanofsky sdaftuar sipa vasild wumpus 19:00:29 <meshcollider> hi 19:00:30 <jonasschnelli> hi 19:00:33 <wumpus> still bot-less i see 19:00:37 <sipa> hi 19:00:39 <jonatack> hi 19:00:39 <MarcoFalke> ahoy 19:00:47 <promag> hello 19:01:02 <luke-jr> who runs the bot? 19:01:28 <wumpus> aj i think? 19:02:17 <wumpus> it looks like there are no proposed meeting topics for this week, any last minute topic proposals? 19:02:27 <emzy> hi 19:02:38 <sipa> what's left before 0.21 fork off? 19:02:42 <MarcoFalke> sipa: review 19:02:45 <luke-jr> whether to do #20250 19:02:49 <gribble> https://github.com/bitcoin/bitcoin/issues/20250 | Bugfix: RPC/Wallet: Make BTC/kB and sat/B fee modes work sanely by luke-jr ÷ Pull Request #20250 ÷ bitcoin/bitcoin ÷ GitHub 19:02:59 <wumpus> https://github.com/bitcoin/bitcoin/issues?q=is%3Aopen+is%3Aissue+milestone%3A0.21.0 19:03:26 <MarcoFalke> https://github.com/bitcoin/bitcoin/milestone/45 has 16 items 19:03:37 <luke-jr> I can rebase (and retitle) it, but I'm not sure it's worth the effort if we don't consider it worth doing 19:04:02 <wumpus> MarcoFalke: eh yes, that link is better, i alrady wondered how it ended up with so little items suddenly 19:04:22 <wumpus> we need to go over the list and decide what is necessary to include in 0.21.0 and what can wait 19:04:22 <luke-jr> wumpus: your link excluded PRs :p 19:04:47 <dongcarl> Random observation: Cirrus is so much better about starting CI tasks on time compared to Travis. Thanks MarcoFalke! 19:05:11 <MarcoFalke> luke-jr: I still think we should do *something*. Whether that is #20250 or #20305 19:05:13 <gribble> https://github.com/bitcoin/bitcoin/issues/20250 | Bugfix: RPC/Wallet: Make BTC/kB and sat/B fee modes work sanely by luke-jr ÷ Pull Request #20250 ÷ bitcoin/bitcoin ÷ GitHub 19:05:14 <gribble> https://github.com/bitcoin/bitcoin/issues/20305 | wallet: introduce fee_rate_sat_vb param/option by jonatack ÷ Pull Request #20305 ÷ bitcoin/bitcoin ÷ GitHub 19:05:41 <MarcoFalke> forcing user to use named args with conf_target=0.0003 seems broken 19:05:41 <wumpus> #20234 seems to be controverial and is still in the discussion phase 19:05:44 <gribble> https://github.com/bitcoin/bitcoin/issues/20234 | net: dont extra bind for Tor if binds are restricted by vasild ÷ Pull Request #20234 ÷ bitcoin/bitcoin ÷ GitHub 19:06:03 <wumpus> so I think removing the milestone there makes sense 19:06:17 <MarcoFalke> wumpus: Agree 19:06:50 <jnewbery> hi 19:06:51 <hebasto> agree too 19:07:08 <aj> hi 19:07:13 <luke-jr> aj: where bot 19:07:20 <wumpus> #20284 was discussed in the P2P meeting, *something* like it needs to go in to make sure that previous versions don't parse the peers.dat as garbage and insert gerbage addresses 19:07:22 <gribble> https://github.com/bitcoin/bitcoin/issues/20284 | addrman: ensure old versions dont parse peers.dat by vasild ÷ Pull Request #20284 ÷ bitcoin/bitcoin ÷ GitHub 19:07:46 <sipa> will review that one soon 19:08:33 <wumpus> I had removed #20205 from the 0.21.0 milestone but jonaschnelli re-added it 19:08:36 <gribble> https://github.com/bitcoin/bitcoin/issues/20205 | wallet: Properly support a wallet id by achow101 ÷ Pull Request #20205 ÷ bitcoin/bitcoin ÷ GitHub 19:08:41 <luke-jr> 20205 is needed 19:08:43 <wumpus> I think it's also contoversial 19:08:57 <luke-jr> wumpus: absolute worst case we'd just not use it 19:08:59 <wumpus> there seems to be no hurry and people differ in opinion whther it's needed at all 19:09:07 <jonasschnelli> wumpus: I didn't know that you have removed it. 19:09:13 <luke-jr> the hurry is to not create wallets with a regression 19:09:25 <luke-jr> all wallets today have a unique id 19:09:28 <wumpus> jonasschnelli: https://github.com/bitcoin/bitcoin/pull/20205#issuecomment-718632332 19:09:31 <jonasschnelli> Adding a UUID later leads probably to a number of wallets without unique ids. 19:09:41 <wumpus> luke-jr: yes, but it is only necessary for bdb, it's unclear if it's necessary in general 19:10:06 <luke-jr> wumpus: even in doubt (which I don't have anyway), it would still make sense to keep it 19:10:07 <wumpus> in any case it is delaying the split-off 19:10:21 <luke-jr> it doesn't have to be, it's trivial to review 19:10:43 <luke-jr> it's not like we're ready to split off anyway 19:10:52 <wumpus> #20318 (thanks for catching this last minute) and #20292 are no-brainers 19:10:53 <gribble> https://github.com/bitcoin/bitcoin/issues/20318 | build: Ensure source tarball has leading directory name by MarcoFalke ÷ Pull Request #20318 ÷ bitcoin/bitcoin ÷ GitHub 19:10:54 <gribble> https://github.com/bitcoin/bitcoin/issues/20292 | test: Fix intermittent feature_taproot issue by MarcoFalke ÷ Pull Request #20292 ÷ bitcoin/bitcoin ÷ GitHub 19:11:29 <meshcollider> #19502 has needed rebase for a few days now, that needs to be rebased or removed from the milestone 19:11:32 <gribble> https://github.com/bitcoin/bitcoin/issues/19502 | Bugfix: Wallet: Soft-fail exceptions within ListWalletDir file checks by luke-jr ÷ Pull Request #19502 ÷ bitcoin/bitcoin ÷ GitHub 19:11:42 <luke-jr> I can reopen my simpler wallet-id PR if 20205 if we want something even easier to review - achow101 didn't like the layer stuff though 19:11:47 <wumpus> #20120 seems mostly test related 19:11:49 <gribble> https://github.com/bitcoin/bitcoin/issues/20120 | net, rpc, test, bugfix: update GetNetworkName, GetNetworksInfo, regression tests by jonatack ÷ Pull Request #20120 ÷ bitcoin/bitcoin ÷ GitHub 19:12:10 <luke-jr> meshcollider: I can do that 19:12:13 <achow101> luke-jr: your pr would have introduced garbage that we would have to keep around forever. it may be simpler, but it definitely is no the correct way 19:12:16 <jnewbery> I don't think we should be adding controversial features last minute to accommodate knots, which appears to be the main motivation for 20205 19:12:23 <wumpus> if there's principal issues with the idea of having a unique id, I don't think opening another PR will resolve that 19:12:44 <jonatack> 20120 is a bugfix with 3-4 acks 19:13:08 <sipa> #20120 19:13:09 <luke-jr> jnewbery: this isn't adding features, it's NOT removign existing feature 19:13:09 <gribble> https://github.com/bitcoin/bitcoin/issues/20120 | net, rpc, test, bugfix: update GetNetworkName, GetNetworksInfo, regression tests by jonatack ÷ Pull Request #20120 ÷ bitcoin/bitcoin ÷ GitHub 19:13:21 <wumpus> #19502 seems to have reviews and ACKs but needs rebase 19:13:23 <gribble> https://github.com/bitcoin/bitcoin/issues/19502 | Bugfix: Wallet: Soft-fail exceptions within ListWalletDir file checks by luke-jr ÷ Pull Request #19502 ÷ bitcoin/bitcoin ÷ GitHub 19:13:35 <wumpus> jonatack: yep 19:14:12 <luke-jr> jnewbery: and Core can absolutely make use of it as well, even if review is slower 19:15:21 <wumpus> #20266 is a straightforward bug fix 19:15:23 <gribble> https://github.com/bitcoin/bitcoin/issues/20266 | wallet: fix change detection of imported internal descriptors by achow101 ÷ Pull Request #20266 ÷ bitcoin/bitcoin ÷ GitHub 19:15:32 <wumpus> and also ACKed 19:16:09 <wumpus> #18836 has many changes and only an approach ACK yet 19:16:11 <gribble> https://github.com/bitcoin/bitcoin/issues/18836 | wallet: upgradewallet fixes and additional tests by achow101 ÷ Pull Request #18836 ÷ bitcoin/bitcoin ÷ GitHub 19:16:34 <aj> ugh, apparently some upgrade broke poor lightningbot 19:17:05 <meshcollider> wumpus: it has an ACK too 19:17:16 <meshcollider> And I am very nearly finished reviewing it 19:17:18 <wumpus> ah no it has two normal ACKs as well 19:17:23 <wumpus> thanks github 19:17:40 <luke-jr> descriptor wallets should never have been merged without unique ids, it's a bug that they're missing 19:17:57 <jonatack> heh github the ack hacker 19:18:07 <wumpus> "hidden items" are for games not SCM interfaces 19:19:06 <wumpus> #20153 is a bugfix and has two ACKs 19:19:08 <gribble> https://github.com/bitcoin/bitcoin/issues/20153 | wallet: do not import a descriptor with hardened derivations into a watch-only wallet by S3RK ÷ Pull Request #20153 ÷ bitcoin/bitcoin ÷ GitHub 19:19:42 <MarcoFalke> 18836 doesn't fix a regression I think, so it is fine to merge or not to merge, depending on review 19:20:30 <wumpus> then there's #18818, which is unnecessary in my opinion, there hasn't been much review otherwise 19:20:33 <gribble> https://github.com/bitcoin/bitcoin/issues/18818 | Fix release tarball generated by gitian by luke-jr ÷ Pull Request #18818 ÷ bitcoin/bitcoin ÷ GitHub 19:21:13 <wumpus> so the biggest thing left for 0.21.0 seems to be the RPC unit discussion 19:21:33 <luke-jr> 18818 is part of 0.20 already 19:21:41 <wumpus> #20305 #20250 19:21:42 <gribble> https://github.com/bitcoin/bitcoin/issues/20305 | wallet: introduce fee_rate_sat_vb param/option by jonatack ÷ Pull Request #20305 ÷ bitcoin/bitcoin ÷ GitHub 19:21:44 <gribble> https://github.com/bitcoin/bitcoin/issues/20250 | Bugfix: RPC/Wallet: Make BTC/kB and sat/B fee modes work sanely by luke-jr ÷ Pull Request #20250 ÷ bitcoin/bitcoin ÷ GitHub 19:21:50 <luke-jr> partially 19:21:54 <wumpus> luke-jr: no, #20318 is 19:21:55 <gribble> https://github.com/bitcoin/bitcoin/issues/20318 | build: Ensure source tarball has leading directory name by MarcoFalke ÷ Pull Request #20318 ÷ bitcoin/bitcoin ÷ GitHub 19:22:21 <wumpus> yes, it is part of it, true 19:23:14 <jonatack> 20305 contains a bugfix for the send rpc that can be moved to its own pull, if needed. The rest is really easier to do pre-release. Afterward, it would have to be overhauled a bit to support both the overloading and the new param. 19:23:15 <MarcoFalke> The autogen.sh part was controversial and is not a regression-bugfix, so I've removed the milestone 19:23:45 <wumpus> regarding the RPC units it's good not to introduce a RPC inconsistency for a release, so I agree we need to do something there 19:24:10 <wumpus> MarcoFalke: +1 19:24:14 <jonatack> If we don't want to overload conf_target and estimate_mode in these 6 RPCs, better to not release them 19:24:31 <jonatack> than to have to support them and then deprecate them 19:24:37 <wumpus> yes 19:25:10 <wumpus> that would be silly 19:25:59 <jonatack> so that was the motivation, sorry for doing it so late, but the merge yesterday of the PR 11413 follow-ups motivated me to spike on it 19:26:44 <luke-jr> MarcoFalke: it is a bugfix for a regression in 0.20, but whatever 19:27:05 <MarcoFalke> luke-jr: Yes, so it is not a regression in 0.21 19:27:49 <wumpus> you're always very quick to call things bugfixes 19:28:05 <luke-jr> wumpus: when they actually are, yes 19:29:06 <sipa> what is the bug here? 19:29:07 <wumpus> in any case we've been over the entire list now--that concludes this topic, happy reviewing 19:29:27 <luke-jr> sipa: building goes looking for .git outside of the source tree and uses whatever it finds 19:29:39 <sipa> ah 19:41:00 <luke-jr> lightningbot: ping 19:41:00 <lightningbot> pong 19:41:44 <luke-jr> aj: thanks 19:42:46 <aj> i don't think it actually supports meetings again yet 20:00:27 <kanzure> timezones. 20:04:20 <luke-jr> kanzure: don't use them 22:23:37 <bitcoin-git> [gui] jarolrod opened pull request #129: qt: Fix Shortcut Ambiguities, Clean up text (master...optionsMenuCleanup) https://github.com/bitcoin-core/gui/pull/129 22:27:48 <bitcoin-git> [bitcoin] stepansnigirev opened pull request #20326: tests: Fix ecdsa_verify in test framework (master...fix-test-ecdsa-verification) https://github.com/bitcoin/bitcoin/pull/20326 00:38:28 <fanquake> Received a response from GitHub in regards to our "load more" issues: https://0bin.net/paste/ox1MH8xR#Kn-kr5erlHeeOnBXYiloYLuikSrO03S+46FEGxLO+Ki. 00:39:12 <fanquake> The email they are responding to I sent on the 12 October. Still haven't received any response to issues opened in the maintainer group in regards to our "load more" or "show/hide" issues. 00:57:45 <fanquake> Actually, we have got a response in regards to the show/hide issue. It's that they cannot reproduce. I assume that's because they waited 5 days to look, and it's since been merged / something has changed. 03:47:50 <wumpus> fanquake: thanks for the update 04:35:51 <luke-jr> #19502 rebased 04:35:53 <gribble> https://github.com/bitcoin/bitcoin/issues/19502 | Bugfix: Wallet: Soft-fail exceptions within ListWalletDir file checks by luke-jr ÷ Pull Request #19502 ÷ bitcoin/bitcoin ÷ GitHub 05:32:08 <wumpus> luke-jr: thanks 05:33:07 <wumpus> fanquake: ideally we could disable hiding of items completely for the entire repository, but i guess that would bring back the unicorns 05:34:16 <wumpus> surprising how fast computers and internet connections are nowadays but showing a moderate number of items of text is still problematic 06:00:36 <fanquake> wumpus: IâÂÂll ask, but yea probably. My understanding is that the hiding was introduced to fix the fact that they canâÂÂt load enough page content at once. 06:48:20 <wumpus> yes, exactly 07:04:51 <bitcoin-git> [bitcoin] MarcoFalke pushed 2 commits to master: https://github.com/bitcoin/bitcoin/compare/f5cdc290d5a4...65460c207c0b 07:04:52 <bitcoin-git> bitcoin/master faf5fa7 MarcoFalke: wallet: Set DatabaseStatus::SUCCESS in MakeSQLiteDatabase 07:04:52 <bitcoin-git> bitcoin/master 65460c2 MarcoFalke: Merge #20324: wallet: Set DatabaseStatus::SUCCESS in MakeSQLiteDatabase 07:05:11 <bitcoin-git> [bitcoin] MarcoFalke merged pull request #20324: wallet: Set DatabaseStatus::SUCCESS in MakeSQLiteDatabase (master...2011-walletSqliteSuccess) https://github.com/bitcoin/bitcoin/pull/20324 07:11:16 <bitcoin-git> [bitcoin] MarcoFalke pushed 2 commits to master: https://github.com/bitcoin/bitcoin/compare/65460c207c0b...c51c2753a4ff 07:11:16 <bitcoin-git> bitcoin/master 568a1d7 Stepan Snigirev: fix ecdsa verify in test framework 07:11:17 <bitcoin-git> bitcoin/master c51c275 MarcoFalke: Merge #20326: tests: Fix ecdsa_verify in test framework 07:11:36 <bitcoin-git> [bitcoin] MarcoFalke merged pull request #20326: tests: Fix ecdsa_verify in test framework (master...fix-test-ecdsa-verification) https://github.com/bitcoin/bitcoin/pull/20326 07:35:32 <bitcoin-git> [bitcoin] fanquake pushed 2 commits to master: https://github.com/bitcoin/bitcoin/compare/c51c2753a4ff...a0c00ff7c045 07:35:32 <bitcoin-git> bitcoin/master 04a69c2 Jonas Schnelli: macOS deploy: use the new plistlib API 07:35:33 <bitcoin-git> bitcoin/master a0c00ff fanquake: Merge #20298: macOS deploy: use the new plistlib API 07:35:51 <bitcoin-git> [bitcoin] fanquake merged pull request #20298: macOS deploy: use the new plistlib API (master...2020/11/macdeployfix) https://github.com/bitcoin/bitcoin/pull/20298 07:54:43 <meshcollider> wumpus: it would be nice and fast if there was a plaintext mode without their fancy formatting 08:00:59 <fanquake> meshcollider: Could you chuck out an opinion in regards to #20305 & #20250. Not sure if they are necessarily in conflict? 08:01:01 <gribble> https://github.com/bitcoin/bitcoin/issues/20305 | wallet: introduce fee_rate_sat_vb param/option by jonatack ÷ Pull Request #20305 ÷ bitcoin/bitcoin ÷ GitHub 08:01:03 <gribble> https://github.com/bitcoin/bitcoin/issues/20250 | Bugfix: RPC/Wallet: Make BTC/kB and sat/B fee modes work sanely by luke-jr ÷ Pull Request #20250 ÷ bitcoin/bitcoin ÷ GitHub 08:01:44 <meshcollider> fanquake: will do soon, thanks 09:15:10 <bitcoin-git> [bitcoin] MarcoFalke opened pull request #20328: cirrus: Skip tasks on the gui repo main branch (master...2011-cirrusSkip) https://github.com/bitcoin/bitcoin/pull/20328 09:20:51 <bitcoin-git> [bitcoin] dgpv opened pull request #20329: docs/descriptors.md: Remove hardened marker in the path after xpub (master...fix-descriptors-md-hardened-after-xpub) https://github.com/bitcoin/bitcoin/pull/20329 10:28:29 <wumpus> meshcollider: agree! 17:08:23 <bitcoin-git> [bitcoin] MarcoFalke pushed 2 commits to master: https://github.com/bitcoin/bitcoin/compare/a0c00ff7c045...4727c1ca2493 17:08:24 <bitcoin-git> bitcoin/master 66667ac MarcoFalke: cirrus: Skip tasks on the gui repo main branch 17:08:24 <bitcoin-git> bitcoin/master 4727c1c MarcoFalke: Merge #20328: cirrus: Skip tasks on the gui repo main branch 17:08:43 <bitcoin-git> [bitcoin] MarcoFalke merged pull request #20328: cirrus: Skip tasks on the gui repo main branch (master...2011-cirrusSkip) https://github.com/bitcoin/bitcoin/pull/20328 17:10:10 <bitcoin-git> [bitcoin] MarcoFalke pushed 3 commits to master: https://github.com/bitcoin/bitcoin/compare/4727c1ca2493...7e373294a5ae 17:10:10 <bitcoin-git> bitcoin/master fac0517 MarcoFalke: travis: Remove s390x build 17:10:11 <bitcoin-git> bitcoin/master fa2c3c0 MarcoFalke: ci: Set LC_ALL=C to allow running the s390x tests in qemu 17:10:12 <bitcoin-git> bitcoin/master 7e37329 MarcoFalke: Merge #20315: travis: Remove s390x build 17:10:30 <bitcoin-git> [bitcoin] MarcoFalke merged pull request #20315: travis: Remove s390x build (master...2010-ciS390x) https://github.com/bitcoin/bitcoin/pull/20315 19:00:10 <achow101> wallet meeting? 19:00:11 <meshcollider> #startmeeting 19:00:18 <meshcollider> #bitcoin -core-dev Wallet 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 ariard digi_james amiti fjahr 19:00:18 <meshcollider> jeremyrubin emilengler jonatack hebasto jb55 kvaciral ariard digi_james amiti fjahr jeremyrubin lightlike emilengler jonatack hebasto jb55 elichai2 19:00:28 <meshcollider> Topics? 19:01:23 <meshcollider> I guess we should discuss the PRs left on the milestone 19:01:43 <achow101> are we going to talk about wallet ids for the 5th meeting in a row? 19:02:11 <meshcollider> I don't think talking about wallet IDs is going to go anywhere... 19:02:20 <jonatack> hi 19:02:38 <meshcollider> I'm more interested in #20250 and #20305 19:02:40 <gribble> https://github.com/bitcoin/bitcoin/issues/20250 | Bugfix: RPC/Wallet: Make BTC/kB and sat/B fee modes work sanely by luke-jr ÷ Pull Request #20250 ÷ bitcoin/bitcoin ÷ GitHub 19:02:41 <gribble> https://github.com/bitcoin/bitcoin/issues/20305 | wallet: introduce fee_rate_sat_vb param/option by jonatack ÷ Pull Request #20305 ÷ bitcoin/bitcoin ÷ GitHub 19:03:15 <jonatack> just pushed an update after re-reviewing and re-testing each commit 19:03:26 <jonatack> (to 20305) 19:04:36 <achow101> I will review those eventually 19:05:23 <meshcollider> I guess the key topic is 1) which of the two (fairly similar) directions do we want to take, and 2) which of the side changes in each PR do we also want in 0.21 19:06:13 <achow101> are they mutually exclusive? 19:06:24 <jonatack> i think the directions are fundamentally different, apart from the feeRate -> fee_rate alias in 20250 that is orthogonal 19:06:42 <jonatack> achow101:: if merged before the release, yes 19:07:17 <jonatack> achow101:: if merged after, then 20305 doesn't work and would need to be overhauled 19:07:45 <jonatack> as it replaces the overloading with the new fixed-unit fee rate param 19:07:47 <achow101> hmm ok. i'll try to get to them today 19:07:55 <achow101> I haven't looked at them yet, so no opinion 19:08:19 <jonatack> and if the overloading is released in 0.21, then we'll have to support it and then deprecate it 19:08:44 <achow101> ngl it feels like we should make an rpc api v2 and just overhaul everything 19:09:15 <jonatack> i think it's workable to improve the current one 19:09:17 <luke-jr> I'm in the middle of rebasing 20250 19:09:17 <achow101> but maybe i'm just in the "blow up everything" mindset 19:09:34 <jonatack> i sort of detail the steps in the PR description 19:09:37 <meshcollider> achow101: I think both are true ;) 19:10:24 <jonatack> luke-jr: ping me when you're ready and i'll review. even if 20305 is merged, i think the commit that does feeRate -> fee_rate is good 19:10:38 <jonatack> i didn't do that since i saw that you proposed it 19:11:02 <meshcollider> I am slightly more in favour of the fixed-unit param, but don't have a big preference 19:11:27 <meshcollider> And yes it's definitely good to have fee_rate consistent 19:11:36 <jonatack> same, per discussion on in #19543 19:11:38 <gribble> https://github.com/bitcoin/bitcoin/issues/19543 | Normalize fee units for RPC ("BTC/kB" and "sat/B) ÷ Issue #19543 ÷ bitcoin/bitcoin ÷ GitHub 19:12:33 <luke-jr> I disliked EXPLICIT ever got split into BTC/kB and sat/B 19:12:36 <jonatack> 20305 implements the conclusion by wumpus at https://github.com/bitcoin/bitcoin/pull/20220#discussion_r513789526 19:13:24 <jonatack> luke-jr: did you see the twitter poll; people prefer sat/vB over btc/kvB by 10 to 1 19:13:53 <meshcollider> Yeah I think sat/vB is definitely the way to go IMO 19:13:57 <meshcollider> it doesn't seem like there are many others here to discuss so let's just leave it as an #action to review and weigh in on the two alternatives 19:14:08 <luke-jr> jonatack: I didn't 19:14:42 <jonatack> https://twitter.com/jonatack/status/1318890833131823104?s=20 19:14:55 <meshcollider> There's a couple of easy wallet PRs left, but the main one on the milestone is #18836 19:14:57 <gribble> https://github.com/bitcoin/bitcoin/issues/18836 | wallet: upgradewallet fixes and additional tests by achow101 ÷ Pull Request #18836 ÷ bitcoin/bitcoin ÷ GitHub 19:14:59 <meshcollider> So please review that 19:15:01 <luke-jr> can we just drop BTC/kB entirely? :x 19:15:09 <jonatack> oh, not 10:1, the ratio is much higher 19:15:20 <meshcollider> luke-jr: fee_rate_sat_vb does that :p 19:15:26 <jonatack> luke-jr: agreed! 19:15:54 <achow101> I might go through 18836 on stream on Monday. The hard part is figuring out the BDB format 19:16:09 <luke-jr> meshcollider: but with an ugly name 19:16:24 <luke-jr> looks like bumpfee is the only thing in 0.20 with "fee_rate" 19:16:27 <luke-jr> hrm 19:16:54 <jonatack> achow101: is 18836 ready for review? if yes, on it tomorrow 19:17:08 <meshcollider> jonatack: yes it's been ready for weeks :p 19:17:10 <achow101> jonatack: yes. it has been for a while 19:17:20 <jonatack> ok ty 19:17:21 <meshcollider> I have been slack personally 19:18:14 <jonatack> luke-jr: yep; i'm thinking we add the ugly named one (at least you won't call it by mistake), then deprecate the btc/kB ones, then alias the ugly one to fee_rate 19:18:28 <jonatack> over a couple releases 19:18:50 <jonatack> maybe with a config option to opt in to the new fee_rate param in sat/vB 19:19:27 <luke-jr> jonatack: that still has a risk 19:19:44 <luke-jr> the only clean solution I see is to use BTC/kB everywhere 19:19:51 <luke-jr> :/ 19:20:08 <jonatack> it seems users really want sat/vb 19:20:21 <luke-jr> well, we've really wanted satoshis for amounts since like 0.3 too 19:20:38 <jonatack> and the risk is a too-low feerate, not a too-high one, and if we put it behind an opt-in config option... 19:20:59 <jonatack> idk seems manageable 19:21:01 <luke-jr> true, and it only affects bumpfee 19:21:16 <luke-jr> so too-low is presumably rejected, and if not you can always replace it with the right one 19:21:18 <jonatack> and fundraw and 19:21:26 <luke-jr> no, fundraw uses feeRate 19:21:29 <jonatack> funded psbt 19:21:42 <jonatack> anyway, it's a limited risk 19:21:56 <luke-jr> only bumpfee uses fee_rate in 0.20 afaict 19:22:18 <luke-jr> I'm going to abandon my rebase and close 20250 then, ok? 19:22:39 <luke-jr> IMO fee_rate_sat_vb should just get renamed to fee_rate 19:23:26 <jonatack> luke-jr: no deprecation process? config option opt-in? 19:23:36 <luke-jr> jonatack: no, not needed considering 19:23:54 <luke-jr> maybe support -deprecatedrpc=fee_rate or something, but meh 19:24:12 <jonatack> i'm ok with remaning if reviewers are 19:24:24 <bitcoin-git> [bitcoin] luke-jr closed pull request #20250: Bugfix: RPC/Wallet: Make BTC/kB and sat/B fee modes work sanely (master...rpcwallet_explicit_fixups) https://github.com/bitcoin/bitcoin/pull/20250 19:25:00 <meshcollider> we should take a bit from 20250 as previously discussed, like the feeRate -> fee_rate though 19:25:15 <jonatack> luke-jr: if 20305 doesn't get in for 0.21 then I think we need 20250, so keep it around 19:25:26 <jonatack> meshcollider: if we don't rename to fee_rate, agreed 19:26:30 <meshcollider> I think with the right "too low" errors in place, just using fee_rate is fine, but I am not going to speak on behalf of all reviewers :p 19:26:37 <luke-jr> meshcollider: that's part of 20305 with my suggestion 19:26:58 <luke-jr> feeRate would continue to be BTC/kB (deprecated) until we remove it entirely 19:27:14 <luke-jr> (though I don't really care if we just remove it right now too) 19:27:42 <jonatack> ok let me know, i'll be active on updating as time is short 19:28:13 <meshcollider> achow101 can weigh in when he reviews 19:28:24 <meshcollider> Alright anything else to discuss today? 19:28:24 <jonatack> feeRate in walletcreatefundedpsbt and bumpfee iirc 19:30:22 <meshcollider> #endmeeting 19:30:35 <achow101> lightningbot still dead :( 19:30:35 <lightningbot> achow101: Error: "still" is not a valid command. 19:31:28 <jonatack> meshcollider: congrats on the john pfeffer support! (just seeing it now) ð 19:31:51 <luke-jr> aj 19:35:08 <meshcollider> jonatack: yeah it's super nice of him! 19:35:25 <meshcollider> achow101: lol the bot doesn't like being called dead 19:36:22 <meshcollider> Unlike usual, when I started the meeting today, I got an automatic list of commands send to me by core-meetingbot 19:36:29 <meshcollider> Maybe that's a WIP 19:36:39 <achow101> ooh 19:37:00 <achow101> maybe we can delegate the pinging to the bot too 19:40:11 <meshcollider> That would be super handy 19:40:14 <jonatack> meshcollider: with you and luke-jr voting for fee_rate_sat_vb -> fee_rate, LMK if I should start updating. Meanwhile, queuing up and building 18836 19:40:38 <meshcollider> achow101: what do you think ^ 19:40:51 <meshcollider> kallewoof might also like to weigh in? 20:59:29 <jonasschnelli> the wallet meeting was logged by my just created meetingbot (core-meetingbot) https://bitcoin.jonasschnelli.ch/ircmeetings/logs/bitcoin-core-dev/2020/bitcoin-core-dev.2020-11-06-19.00.moin.txt 20:59:47 <jonasschnelli> probably lightningbot are also recoding the meetings... 21:01:57 <jonasschnelli> I guess those bots just have no right to post in this channel 21:04:04 <jonasschnelli> Indeed. Lightning bot also recorded the last meeting: http://www.erisian.com.au/meetbot/bitcoin-core-dev/2020/bitcoin-core-dev.2020-11-05-19.00.log.txt (the one from yesterday). 21:04:37 <luke-jr> weird 21:05:03 <jonasschnelli> I tested the meetingbot (core-meetingbot) in ##bitcoin-core-gui and it works there... 21:05:18 <jonasschnelli> so maybe its the channel mode or so or some sort of bot protection. 21:32:29 <achow101> jonasschnelli: but lightning bot responded to pings 21:32:44 <achow101> lightningbot: ping 21:32:44 <lightningbot> pong 00:45:42 <luke-jr> jonasschnelli: why is the gui channel invite-only? 05:24:26 <bitcoin-git> [bitcoin] LarryRuane opened pull request #20331: allow -loadblock blocks to be unsorted (master...loadblock-unsorted) https://github.com/bitcoin/bitcoin/pull/20331 06:12:11 <jonasschnelli> Luke-jr: it should no longer be invite only 06:13:23 <jonasschnelli> achow101: yeah. Wired. Maybe the startmeeting and endmeeting messages are suppressed by the channel/freenode? Some anti spam thing maybe. 06:56:32 <bitcoin-git> [bitcoin] MarcoFalke opened pull request #20332: test: Mock IBD in net_processing fuzzers (master...2011-fuzzNet) https://github.com/bitcoin/bitcoin/pull/20332 11:47:17 <bitcoin-git> [bitcoin] promag closed pull request #19917: Yet another change to reduce recursive mempool locking (master...2020-09-removeunbroadcasttx) https://github.com/bitcoin/bitcoin/pull/19917 13:12:18 <bitcoin-git> [bitcoin] fanquake opened pull request #20333: build: remove native_biplist dependency (master...no_more_biplist) https://github.com/bitcoin/bitcoin/pull/20333 14:24:24 <jonasschnelli> core-meetingbot ping 14:25:00 <jonasschnelli> lightningbot ping 14:25:00 <lightningbot> pong 05:35:29 <sipa> d/query gwillen 05:35:33 <sipa> oops 07:22:54 <bitcoin-git> [bitcoin] fanquake opened pull request #20336: build: automatically determine macOS base translations (master...auto_find_translations) https://github.com/bitcoin/bitcoin/pull/20336 12:15:43 <bitcoin-git> [bitcoin] MarcoFalke opened pull request #20339: ci: Run macos ci config on cirrus (master...2011-moreCirrus) https://github.com/bitcoin/bitcoin/pull/20339 18:17:29 <bitcoin-git> [bitcoin] fanliao1989 opened pull request #20343: Update README.md (master...master) https://github.com/bitcoin/bitcoin/pull/20343 18:33:00 <bitcoin-git> [bitcoin] MarcoFalke closed pull request #20343: Update README.md (master...master) https://github.com/bitcoin/bitcoin/pull/20343 19:00:06 <bitcoin-git> [bitcoin] theStack opened pull request #20344: wallet: fix scanning progress calculation for single block range (master...20201108-wallet-avoid_div_by_zero_on_single_block_rescan) https://github.com/bitcoin/bitcoin/pull/20344 19:58:28 <bitcoin-git> [bitcoin] SatoshiBitcoin opened pull request #20345: Update README.md (master...Version1) https://github.com/bitcoin/bitcoin/pull/20345 19:59:18 <bitcoin-git> [bitcoin] SatoshiBitcoin closed pull request #20345: Update README.md (master...Version1) https://github.com/bitcoin/bitcoin/pull/20345 20:21:22 <andytoshi> achow101: i think there is a bug in CreateTransaction related to bnb .. can you sanity check me before i file a bug 20:21:51 <achow101> andytoshi: what's the bug? 20:21:56 <andytoshi> one sec, typing 20:22:19 <andytoshi> the problem is that when we create txNew for the purpose of getting a tx size to estimate the fee required, in this line https://github.com/bitcoin/bitcoin/blob/master/src/wallet/wallet.cpp#L2940 we selectively add a change output based on whether bnb was used by SelectCoins 20:22:25 <andytoshi> so far so good 20:23:07 <andytoshi> but .. if we use bnb, but we don't get enough fees, and we have subtractfeefromeamount on, we hit this line https://github.com/bitcoin/bitcoin/blob/master/src/wallet/wallet.cpp#L3038 20:23:52 <andytoshi> which says "SelectCoins succeeded, we didn't hit the fee we needed, but that's fine, we can increase the fee by subtracting from outputs. turn off pick_new_inputs and loop again" 20:24:22 <andytoshi> but then, when we do the next loop iteration, we skip SelectCoins and just set bnb_used to false https://github.com/bitcoin/bitcoin/blob/master/src/wallet/wallet.cpp#L2928 20:25:23 <andytoshi> which means that when we recalculate nFeeRequired, we'll get a higher number than we did before. we set nFeeRet to nFeeRequired at the end of the previous iteration. so now this check will fail https://github.com/bitcoin/bitcoin/blob/master/src/wallet/wallet.cpp#L2984 20:25:50 <andytoshi> ..and the whole loop will fail, because we say "well, we didn't hit our fee target, and pick_new_inputs was off, so we must be screwed" 20:26:15 <andytoshi> but actually the problem is just that we didn't compute nFeeRequired consistently from one iteration to the next 20:26:22 <andytoshi> (end of description) 20:26:23 <achow101> how is bnb used but not enough fees? 20:26:41 <luke-jr> ARGH, google has decided "dnsseed.bitcoin.dashjr.org" has deceptive content is now blacklisting all of dashjr.org -.- 20:26:57 <achow101> with subtractfeefromamount, unless the amount is less than the fees? 20:27:09 <andytoshi> lemme take a look 20:27:59 <luke-jr> sipa: bluematt: cdecker: jonasschnelli: petertodd: sjors: emzy: this will probably hit you soon too 20:31:06 <jonatack> luke-jr: confirmed 20:31:26 <jonatack> "Deceptive site ahead. Firefox blocked this page because it may trick you into doing something dangerous like installing software or revealing personal information like passwords or credit cards. Advisory provided by Google Safe Browsing." 20:31:30 <emzy> luke-jr: benause of the dnsseed. It has nothing to do with http... 20:32:24 <emzy> https://dashjr.org/ works vor me in Firefox. 20:33:08 <jonatack> dnsseed.bitcoin.dashjr.org has been reported as a deceptive site. You can report a detection problem -> I filed an report 20:33:25 <achow101> andytoshi: regardless, i'm trying to get at least the effective value pr for the next release so there won't be any such bug as the loop is removed 20:36:14 <emzy> luke-jr: you are sure that they will blacklist all of dashjr.org? I think the http(s) blacklist of dnsseed.bitcoin.dashjr.org is right. 20:36:24 <achow101> luke-jr: is the warning part of chrome or is it some dns thing? 20:36:34 <achow101> or something else? 20:37:01 <jonasschnelli> My DNS seed runs no http site (I guess). 20:37:01 <andytoshi> achow101: cool, thanks 20:37:14 <jonasschnelli> Or does it even matter what runs on the seed machine? 20:37:49 <andytoshi> though there's maybe an 80% chance this is actually some elements weirdness related to how we account for change in coin selection 20:38:27 <emzy> jonasschnelli: the A records you deliver are not your IP addresses anyways. 20:40:22 <emzy> As long as only the whole domain is blacklisted for browsing it is totaly fine. 20:42:01 <emzy> I would argue it is right to do it. Because it could lead to a malicious site. 20:45:19 <andytoshi> achow101: oh i think my issue may have been resolved in https://github.com/bitcoin/bitcoin/pull/17458 20:45:33 <andytoshi> certainly the logic before that PR is way harder to understand 20:46:52 <luke-jr> jonatack: well, the dnsseed domain probably should get flagged - the problem is they're extending it to my entire domain :/ 20:47:11 <luke-jr> achow101: I assume it's part of Google's existing censorship framework 20:47:38 <luke-jr> jonasschnelli: it points at random IPs, which may be running malicious webservers.. 20:59:08 <jonasschnelli> Yes. That. But it has always been that way. 21:37:11 <andytoshi> achow101: i think i'm right, this is a core thing not an elements thing ... https://github.com/bitcoin/bitcoin/blob/7e373294a5ae819099c39d9d03d1f5a311d63cfc/src/wallet/wallet.cpp#L3016-L3019 this comment says "nFeeNeeded should not have changed" 21:37:20 <andytoshi> 20:26 < achow101> how is bnb used but not enough fees? 21:37:44 <andytoshi> what's happening here is that bnb is used and it gives us -some- fees, but if we're subtracting fees from outputs it doesn't bother trying to give us enough 21:38:10 <achow101> I suppose bnb assumes that the output is enough to cover the fees 21:38:17 <achow101> but that may not be the case 21:38:18 <andytoshi> the logic is that we'll fail the nFeesRet >= nFeesNeeded check, but then we'll go around the loop again 21:38:22 <andytoshi> it is the case 21:38:27 <andytoshi> but the code is still wrong 21:39:13 <andytoshi> in my code the output is more than enough to cover the fees, the problem is that we don't subtract enough (or rather, we subtract enough but then incorrectly think we should've subtracted more) 21:39:17 <achow101> do you have a particular test case I can try? 21:39:51 <andytoshi> sorta, i can give you an elements functional test from before #17458 21:39:54 <gribble> https://github.com/bitcoin/bitcoin/issues/17458 | Refactor OutputGroup effective value calculations and filtering to occur within the struct by achow101 ÷ Pull Request #17458 ÷ bitcoin/bitcoin ÷ GitHub 21:39:59 <andytoshi> let me try to extract a bitcoin example 21:40:12 <andytoshi> because i'm pretty confident that the issue has nothingh to do with elements 21:42:14 <achow101> Are you hitting the "Transaction fee and change calculation failed" error? 21:42:37 <andytoshi> yep 21:43:01 <bitcoin-git> [bitcoin] tylerchambers opened pull request #20346: script: modify security-check.py to use "==" instead of "is" for literal comparison (master...literal-comparison-update) https://github.com/bitcoin/bitcoin/pull/20346 21:43:37 <andytoshi> i think, if you have a wallet where bnb can solve exactly (say you have two 10btc inputs and want a 20btc output) and you put subtractfeesfromoutput on, you'll trigger this 21:51:50 <achow101> andytoshi: just tried, and nope 21:54:05 <andytoshi> hmmmm, i don't understand. i'll have to run it myself with a pile of logging added 21:55:28 <andytoshi> can you push a diff somewhere to add a test case? 21:55:36 <andytoshi> since you have one 22:01:26 <andytoshi> can i get log output from unit tests? 22:01:59 <achow101> unit tests make a dir in /tmp that should have logs 22:20:40 <achow101> andytoshi: https://github.com/achow101/bitcoin/commit/2c08a27ea062e4247b3f53bcd296429a4ca9b4a3 22:29:17 <andytoshi> thanks! 22:29:43 <andytoshi> i almost have a unit test, i am just trying to set a non-zero feerate without segfaulting.. 00:49:21 <andytoshi> achow101: https://github.com/bitcoin/bitcoin/issues/20347 i'm not crazy :P 00:49:24 <andytoshi> lol what a waste of a sunday 00:50:10 <sipa> sounds like a very useful sunday 00:53:07 <andytoshi> hehe, i did actually learn a lot about core 01:02:16 <achow101> I think you've run into why everyone is afraid of changing coin selection. the logic is too hard to follow 01:03:06 <sipa> it's probably a good reason to justify changes 01:03:36 <sipa> the existing logic is a painful mess, but at least my impression was that it was very unlikely to actually fuck up 01:04:59 <andytoshi> i mean, this is a real corner case, achow and i weren't sure if we could make it trigger without simulating weird fee behavior, and even then 01:05:49 <andytoshi> what's surprising is that it is a "coin selection" bug which is entirely outside of SelectCoins, it's the retry-loop in CreateTransaction 01:06:13 <achow101> unfortunately CreateTransaction is considered part of coin selection behavior because of the loop 05:33:47 <fanquake> wumpus / sipa: may just want to block kengendron251 05:42:08 <sipa> fanquake: going to wait 05:42:19 <sipa> his last message made it sound like a mistake 08:16:36 <wumpus> what did they do? looking at the profile it's either a spammer or a 12 year old :) 08:17:58 <fanquake> Posted a bit of spammy crap, but now it also seems like they've managed to subscribe to the repo, and can't figure out how to unsubscribe. 08:18:59 <aj> deleted comment from 20317 says "I was on this planet well before the computer" so rules out the 12yo theory? :) 08:25:35 <bitcoin-git> [bitcoin] ajtowns opened pull request #20353: configure: Support -fdebug-prefix-map (master...202011-ccache-debug-prefix) https://github.com/bitcoin/bitcoin/pull/20353 14:07:07 <bitcoin-git> [bitcoin] laanwj pushed 2 commits to master: https://github.com/bitcoin/bitcoin/compare/7e373294a5ae...f70eb51b05de 14:07:07 <bitcoin-git> bitcoin/master faa2f06 MarcoFalke: scripted-diff: [build] Ensure source tarball has leading directory name 14:07:08 <bitcoin-git> bitcoin/master f70eb51 Wladimir J. van der Laan: Merge #20318: build: Ensure source tarball has leading directory name 14:07:36 <bitcoin-git> [bitcoin] laanwj merged pull request #20318: build: Ensure source tarball has leading directory name (master...2011-buildTar) https://github.com/bitcoin/bitcoin/pull/20318 14:15:26 <bitcoin-git> [bitcoin] laanwj pushed 2 commits to master: https://github.com/bitcoin/bitcoin/compare/f70eb51b05de...663fd92b28c3 14:15:26 <bitcoin-git> bitcoin/master bd93fc9 Andrew Chow: Fix change detection of imported internal descriptors 14:15:27 <bitcoin-git> bitcoin/master 663fd92 Wladimir J. van der Laan: Merge #20266: wallet: fix change detection of imported internal descriptor... 14:15:46 <bitcoin-git> [bitcoin] laanwj merged pull request #20266: wallet: fix change detection of imported internal descriptors (master...fix-desc-change) https://github.com/bitcoin/bitcoin/pull/20266 14:33:29 <bitcoin-git> [bitcoin] MarcoFalke pushed 6 commits to master: https://github.com/bitcoin/bitcoin/compare/663fd92b28c3...05aeeee34f15 14:33:29 <bitcoin-git> bitcoin/master fafce1a MarcoFalke: ci: Move documentation to correct config file 14:33:30 <bitcoin-git> bitcoin/master fa0795f MarcoFalke: ci: Replace TRAVIS_OS_NAME with CI_OS_NAME 14:33:31 <bitcoin-git> bitcoin/master fa8b111 MarcoFalke: ci: Run arm ci config on cirrus 14:33:48 <bitcoin-git> [bitcoin] MarcoFalke merged pull request #20339: ci: Run more ci configs on cirrus (master...2011-moreCirrus) https://github.com/bitcoin/bitcoin/pull/20339 14:47:40 <bitcoin-git> [bitcoin] laanwj pushed 6 commits to master: https://github.com/bitcoin/bitcoin/compare/05aeeee34f15...4fd37d0a104f 14:47:40 <bitcoin-git> bitcoin/master fa762a3 MarcoFalke: test: Remove unused unnamed parameter from block.serialize call 14:47:41 <bitcoin-git> bitcoin/master fa1dea1 MarcoFalke: test: Fix deser issue in create_block 14:47:42 <bitcoin-git> bitcoin/master fac865b MarcoFalke: test: Fix intermittent feature_taproot issue 14:47:58 <bitcoin-git> [bitcoin] laanwj merged pull request #20292: test: Fix intermittent feature_taproot issue (master...2011-testFixes) https://github.com/bitcoin/bitcoin/pull/20292 14:59:13 <bitcoin-git> [bitcoin] MarcoFalke opened pull request #20354: test: Add feature_taproot.py --previous_release (master...2010-testFeatureTaprootPreviousVersion) https://github.com/bitcoin/bitcoin/pull/20354 15:36:21 <bitcoin-git> [bitcoin] practicalswift opened pull request #20355: fuzz: Check for addrv1 compatibility before using addrv1 serializer/deserializer on CSubNet (master...fix-sub_net_deserialize) https://github.com/bitcoin/bitcoin/pull/20355 16:08:04 <bitcoin-git> [bitcoin] laanwj pushed 4 commits to master: https://github.com/bitcoin/bitcoin/compare/4fd37d0a104f...79a3b59cc706 16:08:05 <bitcoin-git> bitcoin/master ba8997f Jon Atack: net: update GetNetworkName() with all enum Network cases 16:08:05 <bitcoin-git> bitcoin/master 9a75e1e Jon Atack: rpc: update GetNetworksInfo() to not return unsupported networks 16:08:05 <bitcoin-git> bitcoin/master 7b5bd31 Jon Atack: test: add getnetworkinfo network name regression tests 16:08:24 <bitcoin-git> [bitcoin] laanwj merged pull request #20120: net, rpc, test, bugfix: update GetNetworkName, GetNetworksInfo, regression tests (master...fix-getnetworkinfo-empty-networks) https://github.com/bitcoin/bitcoin/pull/20120 17:54:46 <real_or_random> can we reopen https://github.com/bitcoin/bitcoin/issues/17802 ? I think this needs action before the end of the year. 17:55:27 <sipa> MarcoFalke: ping ^ 18:12:09 <az0re> Hey all, just want to point out the totally broken fee estimator 18:12:12 <az0re> !mempool 18:12:12 <gribble> Mempool info: [txcount: 3127, vsize (MB): 7.3364238739, totalfee (BTC): 1.01938529] | Next block 0: [max fee: 12773.8128079, min fee: 1.00305810398] | Next block 1: [max fee: 1.00303951368, min fee: 1.0013713851] | Next block 2: [max fee: 1.00137059849, min fee: 1.001347601] 18:12:37 <az0re> My current mempool histogram shows: 18:12:39 <az0re> [[236, 101197], [205, 109178], [15, 123943], [8, 135543], [1, 7467948]] 18:13:10 <az0re> This is while the mempool has deflated from ~40MB 12 hours ago to ~8MB now 18:13:14 <sipa> who runs gribble these days? 18:13:18 <sipa> nanotube? 18:14:01 <az0re> Not sure 18:14:09 <az0re> Here's what estimatesmartfee gives me: 18:14:11 <az0re> user@host:~$ ./bitcoin-0.20.1/bin/bitcoin-cli -rpcuser=user --rpcpassword=... estimatesmartfee 1 CONSERVATIVE 18:14:12 <az0re> { 18:14:12 <az0re> "feerate": 0.00231653, 18:14:12 <az0re> "blocks": 2 18:14:12 <az0re> } 18:14:13 <az0re> user@host:~$ ./bitcoin-0.20.1/bin/bitcoin-cli -rpcuser=user --rpcpassword=... estimatesmartfee 10 CONSERVATIVE 18:14:18 <az0re> { 18:14:21 <az0re> "feerate": 0.00231653, 18:14:22 <az0re> "blocks": 10 18:14:24 <az0re> } 18:14:26 <az0re> user@host:~$ ./bitcoin-0.20.1/bin/bitcoin-cli -rpcuser=user --rpcpassword=... estimatesmartfee 10 ECONOMICAL 18:14:29 <az0re> { 18:14:31 <az0re> "feerate": 0.00013714, 18:14:33 <az0re> "blocks": 10 18:14:35 <az0re> } 18:14:37 <az0re> user@host:~$ ./bitcoin-0.20.1/bin/bitcoin-cli -rpcuser=user --rpcpassword=... estimatesmartfee 1 ECONOMICAL 18:14:41 <az0re> { 18:14:42 <az0re> "feerate": 0.00199988, 18:14:45 <az0re> "blocks": 2 18:14:49 <az0re> } 18:15:08 <az0re> both ECONOMICAL and CONSERVATIVE suggest *way* too high fees, and I suspect that estimatesmartfee is creating its own reality here: It's suggesting high fees, and so the mempool gets a bunch of really high fee txes, and so the estimator keeps suggesting really high fees 18:15:20 <az0re> However, txes are clearing at way, way lower feerates 18:15:50 <sipa> az0re: that's kind of expected; the estimator does not look at the mempool, but only at the rates of which feerates confirm 18:15:51 <az0re> And if all installed estimators suddenly cut their suggested feerates 10x I suspect nothing would change but people saving a TON on tx fees 18:16:06 <sipa> so it's inherently delayed 18:16:23 <az0re> sipa: Looking at the rates of which feerates confirm should *also* result in a much lower suggested fee 18:16:57 <az0re> In the last 12h I've cleared--in one block--some txes with feerates as low as 3 sat/vbyte 18:18:41 <sipa> interesting 18:19:40 <az0re> There is more historical mempool/feerate estimate data available here: https://github.com/0e319b6/feerate-estimate-data 18:19:51 <az0re> This is far from the only time I've noticed wildly excessive estimated fees 18:20:56 <az0re> !fees 18:20:57 <gribble> Fee estimates (blocks: fee): (2: 68.414),(4: 8.898), (6: 3.319), (10: 1.138), (20: 1.138), (144: 1.138) 18:21:36 <az0re> OK, no idea how these estimates are calculated, but the super sharp drop off is a red flag 18:22:44 <sipa> may be worth opening an issue for 18:37:00 <darosior> az0re: it's expected. It's possible, but there are not historically enough of them for the estimator to choose this bucket. 18:38:06 <darosior> For example, if you had a new mode -say RECKLESS- with far less probability than ECONOMICAL, then it'd probably give you these estimations. 18:53:22 <az0re> darosior: "not historically enough of them" -- of what? 18:59:51 <phantomcircuit> az0re, the problem is that setting the fee rate too high is a known cost, setting it too low tends to result in people with transactions that are stuck 19:00:00 <phantomcircuit> the latter being much worse for 90% of people 19:00:36 <az0re> phantomcircuit: I totally understand that, and I totally understand being conservative. However: 19:00:55 <az0re> 1. Isn't this why there is CONSERVATIVE and ECONOMICAL? `1 ECONOMICAL` in this case really should not be giving me 200 sat/vbyte! 19:01:24 <az0re> 2. We should still see a smooth drop off. But `1 CONSERVATIVE` and `10 CONSERVATIVE` are identical. 19:01:48 <az0re> And it extends beyond 1 and 10; very frequently I see huge swathes of the estimates sitting at exactly the same feerate 19:02:32 <az0re> If `1 CONSERVATIVE` gave 200, `10 CONSERVATIVE` gave 40, and `1 ECONOMICAL` gave 20 I would have nothing to complain about 19:04:35 <az0re> <phantomcircuit> [...] result in people with transactions that are stuck 19:04:43 <az0re> Also, isn't this why RBF is a thing? :) 19:05:29 <az0re> I understand that doesn't necessarily solve the problem in all cases, but it should reduce the caution for suggesting too-low feerates, especially for ECONOMICAL 19:07:09 <az0re> Finally, I will stop spamming after this I swear, but the bimodal distribution of the mempool seems fundamentally broken to me. In my idealized vision of the world, there would be monotonically increasing mempool pressure with decreasing feerate. 19:07:42 <az0re> The current structure suggests to me that the auction mechanics of the mempool are not really working correctly 19:14:04 <queip> long term, would bitcoind start with 1s/vb and then in background keep slowly RBFing it until it works? In transaction you would specify how much you want to wait, how much you can wait, and absolute limit (so before it it sets very max fee) and the max fee 19:15:34 <queip> e.g. dust consolidations would be like 144,1440,6000, 100[s/vb] while regular onchain shopping could be 3,6,24,1000 for example 19:19:42 <bitcoin-git> [bitcoin] laanwj pushed 2 commits to master: https://github.com/bitcoin/bitcoin/compare/79a3b59cc706...1dfe19e2840b 19:19:42 <bitcoin-git> bitcoin/master 538be42 Ivan Metlushko: wallet: fix importdescriptor silent fail 19:19:43 <bitcoin-git> bitcoin/master 1dfe19e Wladimir J. van der Laan: Merge #20153: wallet: do not import a descriptor with hardened derivations... 19:19:57 <bitcoin-git> [bitcoin] laanwj merged pull request #20153: wallet: do not import a descriptor with hardened derivations into a watch-only wallet (master...importdesc_silent_fail) https://github.com/bitcoin/bitcoin/pull/20153 19:20:42 <nanotube> sipa: yes, i still run gribble. mempool command fetches data from mempool.space 19:22:17 <nanotube> az0re: fees command pulls from blockstream.info api 20:13:51 <bitcoin-git> [bitcoin] hebasto opened pull request #20357: ci: Use travis_fold on Travis CI only (master...201109-travis) https://github.com/bitcoin/bitcoin/pull/20357 20:29:32 <kanzure> are we using git read-tree or subtree for libsecp256k1? 20:30:30 <sipa> git subtree; i don't know what git read-tree is 20:30:48 <kanzure> is the usage of subtree documented? i'm thinking about extracting test_framework into a python package. 20:30:58 <kanzure> and i would like to preserve commit history if possible 20:32:21 <luke-jr> that's the opposite direction.. 20:33:10 <sipa> subtree can preserve history afaik 20:33:14 <sipa> when splitting off a tree 20:35:19 <kanzure> opposite direction should be ok 20:36:21 <sipa> git subtree split may do what you want 20:39:33 <bitcoin-git> [bitcoin] ffontaine opened pull request #20358: src/randomenv.cpp: fix build on uclibc (master...master) https://github.com/bitcoin/bitcoin/pull/20358 22:01:30 <bitcoin-git> [bitcoin] dongcarl opened pull request #20359: depends: Various config.site.in improvements and linting (master...2020-11-config-site-cleanup) https://github.com/bitcoin/bitcoin/pull/20359 23:30:44 <stevenroose> I'm reading "A heap is used so that not all items need sorting if only a few are being sent." in net_processing.cpp. 23:31:05 <stevenroose> I don't understand how the C++ heap implementation works I think, does it order lazily? 23:31:54 <sipa> stevenroose: the STL implementation shouldn't matter 23:32:22 <sipa> it constructs a heap in O(n) time from the input elements 23:32:38 <sipa> then you can extract the top element from it in O(log n) time 23:34:03 <sipa> so if you only need m elements, it's O(n + m*log(n)) work 23:34:17 <sipa> while full sorting would need O(n*log(n)) work 23:45:37 <stevenroose> oooh, I'm reading that's just the way a heap works, didn't know that. didn't know it had those properties, fancy 23:46:10 <sipa> yeah, it actually more of a priority queue algorithm than a sorting algorithm 23:46:55 <sipa> heapsort is first structuring the input into a heap, and then iteratively extracting the top level, shrinking the heap, and using the new space to put the extracted element 23:47:01 <sipa> *top element 23:50:46 <meshcollider> real_or_random: done ^ 23:54:22 <luke-jr> wonders where the generational gap is between "computers take time to do things" and "wtf? why isn't it done yet?" 23:56:35 <stevenroose> sipa: thanks 23:59:00 <luke-jr> (I didn't learn to care about optimisation until maybe ~18yo or so) 00:17:40 <ariard> sipa: I guess #20207 is pretty ready modulo jonatack nits :) 00:17:42 <gribble> https://github.com/bitcoin/bitcoin/issues/20207 | Follow-up extra comments on taproot code and tests by sipa ÷ Pull Request #20207 ÷ bitcoin/bitcoin ÷ GitHub 06:58:33 <bitcoin-git> [bitcoin] MarcoFalke pushed 3 commits to master: https://github.com/bitcoin/bitcoin/compare/1dfe19e2840b...0b69bb90eed3 06:58:33 <bitcoin-git> bitcoin/master 0e3a78a practicalswift: fuzz: Check for addrv1 compatibility before using addrv1 serializer/deseri... 06:58:33 <bitcoin-git> bitcoin/master 79b8f8d practicalswift: fuzz: Assert roundtrip equality for both addrv1 and addrv2 versions of CSe... 06:58:34 <bitcoin-git> bitcoin/master 0b69bb9 MarcoFalke: Merge #20355: fuzz: Check for addrv1 compatibility before using addrv1 ser... 06:58:52 <bitcoin-git> [bitcoin] MarcoFalke merged pull request #20355: fuzz: Check for addrv1 compatibility before using addrv1 serializer/deserializer on CSubNet (master...fix-sub_net_deserialize) https://github.com/bitcoin/bitcoin/pull/20355 07:37:23 <bitcoin-git> [bitcoin] mateusz-klatt opened pull request #20361: load wallets from entropy (as BIP39) (master...master) https://github.com/bitcoin/bitcoin/pull/20361 08:22:39 <bitcoin-git> [bitcoin] MarcoFalke pushed 2 commits to master: https://github.com/bitcoin/bitcoin/compare/0b69bb90eed3...42f950cb27b7 08:22:39 <bitcoin-git> bitcoin/master 4444128 MarcoFalke: test: Fix intermittent issue in wallet_listsinceblock 08:22:40 <bitcoin-git> bitcoin/master 42f950c MarcoFalke: Merge #20322: test: Fix intermittent issue in wallet_listsinceblock 08:23:00 <bitcoin-git> [bitcoin] MarcoFalke merged pull request #20322: test: Fix intermittent issue in wallet_listsinceblock (master...2011-testInt) https://github.com/bitcoin/bitcoin/pull/20322 10:13:35 <jonasschnelli> core-meetingbot ping 12:34:41 <MarcoFalke> jonasschnelli: core-meetingbot needs to be logged in with an freenode account to say something here 12:35:24 <MarcoFalke> If you run whois it wounld need to say "core-meetingbot is logged in as core-meetingbot." 12:35:29 <MarcoFalke> *would 12:58:23 <jonasschnelli> MarcoFalke: thanks... IâÂÂll try that. 13:33:39 <jonasschnelli> core-meetingbot ping 13:33:39 <core-meetingbot> pong 13:33:42 <jonasschnelli> #startmeeting 13:33:42 <core-meetingbot> Meeting started Tue Nov 10 13:33:42 2020 UTC. The chair is jonasschnelli. Information about MeetBot at https://bitcoin.jonasschnelli.ch/ircmeetings. 13:33:42 <core-meetingbot> Available commands: action commands idea info link nick 13:33:45 <jonasschnelli> #endmeeting 13:33:45 <core-meetingbot> topic: Bitcoin Core development discussion and commit log | Feel free to watch, but please take commentary and usage questions to #bitcoin | Channel logs: http://www.erisian.com.au/bitcoin-core-dev/, http://gnusha.org/bitcoin-core-dev/ | Meeting topics http://gnusha.org/bitcoin-core-dev/proposedmeetingtopics.txt / http://gnusha.org/bitcoin-core-dev/proposedwalletmeetingtopics.txt 13:33:45 <core-meetingbot> Meeting ended Tue Nov 10 13:33:45 2020 UTC. 13:33:45 <core-meetingbot> Minutes: https://bitcoin.jonasschnelli.ch/ircmeetings/logs/bitcoin-core-dev/2020/bitcoin-core-dev.2020-11-10-13.33.moin.txt 13:35:34 <jonasschnelli> ignore my test-meeting... 14:39:32 <bitcoin-git> [bitcoin] MarcoFalke opened pull request #20362: test: Implicitly sync after generate* to preempt races and intermittent test failures (master...2011-noSync) https://github.com/bitcoin/bitcoin/pull/20362 17:19:12 <DeanWeen> https://gitlab.com/bitcoin-cash-node/bitcoin-cash-node/-/merge_requests/844 In case they "forget" to alert core 17:51:43 <sipa> jnewbery: so if forward/backward compatibility is something to be considered from the perspective of software, it explains my confusion about it: forward compatibility is about making sure downgrading works, backward compatbility is about making sure upgrading works... 18:52:33 <bitcoin-git> [bitcoin] MarcoFalke pushed 2 commits to master: https://github.com/bitcoin/bitcoin/compare/42f950cb27b7...fa8dd34e918c 18:52:33 <bitcoin-git> bitcoin/master fa4234d MarcoFalke: test: Mock IBD in net_processing fuzzers 18:52:34 <bitcoin-git> bitcoin/master fa8dd34 MarcoFalke: Merge #20332: test: Mock IBD in net_processing fuzzers 18:52:53 <bitcoin-git> [bitcoin] MarcoFalke merged pull request #20332: test: Mock IBD in net_processing fuzzers (master...2011-fuzzNet) https://github.com/bitcoin/bitcoin/pull/20332 19:43:19 <jnewbery> sipa: that's my understanding, yes 21:46:19 <bitcoin-git> [bitcoin] troygiorshev opened pull request #20364: Follow-ups to 19107 (master...2020-11-19107-follow-ups) https://github.com/bitcoin/bitcoin/pull/20364 00:04:55 <brianddk> I was trying to build the first few versions of bitcoin (v0.1.5) with MSYS/MinGW (v1.0.11), but I'm failing on the OpenSSL build after applying the readme.txt patches. Anyone recall anything tricky to getting that to work? 00:11:58 <sipa> pretty sure 0 00:12:08 <sipa> pretty sure 0.1.5 was only ever intended to work with msvc 00:12:47 <sipa> oh, no! 00:21:26 <brianddk> sipa, yeah the readme lists some build problems with MSVC, and seems to suggest MinGW/MSYS. I'll keep digging 01:30:41 <bitcoin-git> [bitcoin] jonatack closed pull request #20305: wallet: introduce fee_rate sat/vB param/option (master...fee_rate_sat_vb) https://github.com/bitcoin/bitcoin/pull/20305 01:30:56 <bitcoin-git> [bitcoin] jonatack reopened pull request #20305: wallet: introduce fee_rate sat/vB param/option (master...fee_rate_sat_vb) https://github.com/bitcoin/bitcoin/pull/20305 05:02:20 <bitcoin-git> [bitcoin] S3RK opened pull request #20365: wallettool: add parameter to create descriptors wallet (master...wallet_tool_create_descriptors) https://github.com/bitcoin/bitcoin/pull/20365 05:27:14 <luke-jr> hey, at least the new Cirrus CI work even if the old/first one doesn't <.< 07:36:13 <bitcoin-git> [bitcoin] hebasto opened pull request #20366: scripted-diff: Fix includes when secp256k1 is locally installed on macOS (master...201111-incl) https://github.com/bitcoin/bitcoin/pull/20366 08:12:33 <bitcoin-git> [bitcoin] MarcoFalke opened pull request #20368: ci: Remove redundant valgrind fuzz task (master...2011-ciRemoveHeavyTask) https://github.com/bitcoin/bitcoin/pull/20368 09:11:25 <vasild> sipa: jnewbery: "backward compatibility" can also be applied to data formats, not just on software. For example wikipedia mentions: "A data format is said to be backward compatible with its predecessor if..." (https://en.wikipedia.org/wiki/Backward_compatibility) 09:14:16 <vasild> jnewbery: according to your definition "software is forward-compatible if it can gracefully read new input formats" a software version X is forward compatible until version e.g. X+5 introduces a new input format which X cannot read 09:15:54 <vasild> So the same version X can be forward compatible or not, depending on what future versions do, hmm... 09:23:40 <jnewbery> in that case we'd say that version X+5 breaks forward compatibility 10:49:25 <vasild> jnewbery: wrt https://github.com/bitcoin/bitcoin/pull/20284#issuecomment-725341208 I guess you have verified that it would fix the link error? 13:19:52 <bitcoin-git> [bitcoin] MarcoFalke opened pull request #20370: fuzz: Permission flags in net_processing fuzzers (master...2011-fuzzNetFlags) https://github.com/bitcoin/bitcoin/pull/20370 13:23:34 <bitcoin-git> [bitcoin] MarcoFalke pushed 2 commits to master: https://github.com/bitcoin/bitcoin/compare/fa8dd34e918c...155bf91c3b66 13:23:35 <bitcoin-git> bitcoin/master fa92cf2 MarcoFalke: ci: Remove redundant valgrind fuzz task 13:23:35 <bitcoin-git> bitcoin/master 155bf91 MarcoFalke: Merge #20368: ci: Remove redundant valgrind fuzz task 13:23:54 <bitcoin-git> [bitcoin] MarcoFalke merged pull request #20368: ci: Remove redundant valgrind fuzz task (master...2011-ciRemoveHeavyTask) https://github.com/bitcoin/bitcoin/pull/20368 13:57:50 <jnewbery> vasild: very strange. It builds locally for me. One cirrus task links successfully on that commit and another doesn't 13:57:58 <jnewbery> success: https://cirrus-ci.com/task/5345273754943488 13:58:05 <jnewbery> fail: https://cirrus-ci.com/task/6471173661786112 13:58:20 <vasild> yes, strange and everything compiles for me 13:59:02 <jnewbery> I don't know the cirrus config well enough to know why linking would fail on task 2 13:59:39 <vasild> my guess is that it is running gcc 2.96 on redhat 14:01:49 <jnewbery> it's clang: https://cirrus-ci.com/task/6471173661786112?command=ci#L3080 14:02:14 <jnewbery> platform is ubuntu:focal 14:05:37 <jnewbery> it makes me unhappy to merge stuff that we don't understand. I'd prefer just to remove the FILE_FORMAT from that strprintf than add the declaration in addrman.cpp 14:07:38 <vasild> I wonder if that would compile... 14:07:59 <jnewbery> I mean just don't include that in the exception string 14:08:38 <vasild> yes, I got your point, but I am not sure that will compile 14:09:10 <vasild> it is unclear which version of clang is it using, but I see "setting up clang10" - at least it installed clang10 14:09:18 <vasild> let me try locally with clang10... 14:09:25 <jnewbery> why wouldn't it? That's the line that's causing link failure, or do you think it'd fail somewhere else too? 14:10:00 <vasild> yes, see this one: https://cirrus-ci.com/task/6471173661786112?command=ci#L3847 14:11:26 <vasild> git show 2ed531:src/addrman.h 14:11:40 <vasild> addrman.h:358 14:21:17 <jnewbery> Thanks. That's interesting 14:28:51 <jnewbery> vasild: I wonder if this would work: https://github.com/jnewbery/bitcoin/commit/9efadfb942464bba0e5a000fefaa9f4b0a22b8ed 14:31:19 <MarcoFalke> building locally right now 14:31:44 <MarcoFalke> it is using focal clang with -stdlib=libc++ (depends build) 14:32:39 <vasild> I don't like it - those constants really belong to private members of CAddrMan, moving them away just to fix a compile error that can also be fixed by 1-liner (define the constant in addrman.cpp)... 14:33:15 <vasild> I can reproduce the error locally with clang 10 14:33:20 <MarcoFalke> I think it is a style-question that shoulndn't hold up the release 14:33:58 <vasild> +1 14:36:08 <jnewbery> vasild: can you explain what this line does: https://github.com/bitcoin/bitcoin/pull/20284/files#diff-49d1faa58beca1ee1509a247e0331bb91f8604e30a483a7b2dea813e6cea02e2R74 ? I don't understand how it fixes the build error 14:37:41 <vasild> it defines storage for the variable, my guess is that this particular compiler mistreats constexpr 14:38:36 <vasild> wait, I am trying something simpler... 14:38:46 <vasild> now that I can reproduce locally 14:46:21 <bitcoin-git> [bitcoin] practicalswift opened pull request #20372: Avoid signed integer overflow when loading a mempool.dat file with a malformed time field (master...load-mempool-time-integer-overflow) https://github.com/bitcoin/bitcoin/pull/20372 14:46:38 <vasild> jnewbery: https://bpa.st/MJBA 14:46:46 <vasild> that smells like a compiler bug 14:47:52 <vasild> uint8_t x; ... use x like static_cast<uint8_t>(x) compiles, but change the usage to just "x" without the cast and it does not compile 14:48:45 <vasild> surely the cast from uint8_t to uint8_t shouldn't make a difference 14:50:04 <vasild> Do we want the topmost fix from https://bpa.st/MJBA ? It is line line, but MarcoFalke will have to give up the CustomUintFormatter :) 14:50:18 <MarcoFalke> sob 14:50:20 <vasild> *It is one line 14:50:38 <MarcoFalke> Go ahead. We can fight the compiler after branch off 14:50:59 <vasild> and there is a chance that even though it compiled locally for me with clang 10.0.1 it will break on CI... 14:51:33 <vasild> jnewbery: ? 14:52:15 <jnewbery> looks good to me! 14:52:26 <jnewbery> good job tracking it down 14:54:40 <aj> "for constexpr (but not for const) you always have to provide out-of-class definition on top of the in-class initializer." per https://stackoverflow.com/questions/22172789/passing-a-static-constexpr-variable-by-universal-reference 15:00:16 <bitcoin-git> [bitcoin] MarcoFalke pushed 2 commits to master: https://github.com/bitcoin/bitcoin/compare/155bf91c3b66...179ece42734a 15:00:16 <bitcoin-git> bitcoin/master fa949b3 MarcoFalke: test: Suppress epoll_ctl data race 15:00:17 <bitcoin-git> bitcoin/master 179ece4 MarcoFalke: Merge #20218: test: Suppress epoll_ctl data race 15:00:36 <bitcoin-git> [bitcoin] MarcoFalke merged pull request #20218: test: Suppress epoll_ctl data race (master...2010-testSuppWalletRace) https://github.com/bitcoin/bitcoin/pull/20218 15:12:20 <bitcoin-git> [bitcoin] MarcoFalke pushed 2 commits to master: https://github.com/bitcoin/bitcoin/compare/179ece42734a...d9f5132736f3 15:12:21 <bitcoin-git> bitcoin/master 5e14602 Sebastian Falbesoner: wallet: fix scanning progress calculation for single block range 15:12:22 <bitcoin-git> bitcoin/master d9f5132 MarcoFalke: Merge #20344: wallet: fix scanning progress calculation for single block r... 15:12:40 <bitcoin-git> [bitcoin] MarcoFalke merged pull request #20344: wallet: fix scanning progress calculation for single block range (master...20201108-wallet-avoid_div_by_zero_on_single_block_rescan) https://github.com/bitcoin/bitcoin/pull/20344 15:14:02 <MarcoFalke> vasild: I am running the ci tsan config locally with your "static_cast<uint8_t>(" patch, but it doesn't compile 15:15:54 <vasild> it is on top of 2ed531 15:16:32 <vasild> did you apply it on top of the latest PR 204678c, or on top of 2ed531? 15:18:14 <MarcoFalke> commit 204678cf4258bd59040016a7aac79e05daf30d3c 15:19:12 <vasild> on top of that commit the fix is: https://bpa.st/ASDA 15:19:35 <vasild> I have prepared that and am about to push -f, but will wait for you to confirm that it compiles at your end 15:19:51 <vasild> aj: that is only if the member variable is odr-used 15:20:26 <vasild> the static cast makes it non-odr-used, so it needs no out-of-class definition 15:21:09 <MarcoFalke> ah that seems to compile 15:22:06 <vasild> pushed! 15:23:10 <MarcoFalke> Using uint8_t{FORMAT} also compiles 17:14:11 <bitcoin-git> [bitcoin] hebasto opened pull request #20373: refactor, net: Increase CNode data member encapsulation (master...201111-cnode) https://github.com/bitcoin/bitcoin/pull/20373 19:46:25 <bitcoin-git> [gui] hebasto closed pull request #107: Fix main window geometry save/restore (master...201023-geometry) https://github.com/bitcoin-core/gui/pull/107 20:16:00 <bitcoin-git> [bitcoin] thomasdane opened pull request #20374: Update README.md (master...patch-1) https://github.com/bitcoin/bitcoin/pull/20374 21:52:53 <bitcoin-git> [bitcoin] practicalswift closed pull request #20137: tests: Update UBSan suppressions file with suppressions needed for clang 12 (current trunk) (master...clang-12-ubsan-suppressions) https://github.com/bitcoin/bitcoin/pull/20137 22:01:44 <bitcoin-git> [bitcoin] fanquake closed pull request #20374: Update README.md (master...patch-1) https://github.com/bitcoin/bitcoin/pull/20374 22:37:51 <bitcoin-git> [bitcoin] practicalswift opened pull request #20375: fuzz: Improve coverage for CPartialMerkleTree fuzzing harness (master...fuzzers-2020-11-11) https://github.com/bitcoin/bitcoin/pull/20375 00:27:14 <bitcoin-git> [bitcoin] meshcollider pushed 3 commits to master: https://github.com/bitcoin/bitcoin/compare/d9f5132736f3...c2d8ba6265a4 00:27:15 <bitcoin-git> bitcoin/master 69f59af Luke Dashjr: Bugfix: Wallet: Soft-fail exceptions within ListWalletDir file checks 00:27:15 <bitcoin-git> bitcoin/master 24d2d33 Luke Dashjr: QA: wallet_multiwallet: Check that recursive symlink directory and wallet.... 00:27:16 <bitcoin-git> bitcoin/master c2d8ba6 Samuel Dobson: Merge #19502: Bugfix: Wallet: Soft-fail exceptions within ListWalletDir fi... 00:28:09 <bitcoin-git> [bitcoin] meshcollider merged pull request #19502: Bugfix: Wallet: Soft-fail exceptions within ListWalletDir file checks (master...bugfix_listwalletdir_errors) https://github.com/bitcoin/bitcoin/pull/19502 07:52:51 <hebasto> it seems merged #19502 resolves all issues of #18095, so the latter could be closed. 07:52:53 <gribble> https://github.com/bitcoin/bitcoin/issues/19502 | Bugfix: Wallet: Soft-fail exceptions within ListWalletDir file checks by luke-jr ÷ Pull Request #19502 ÷ bitcoin/bitcoin ÷ GitHub 07:52:55 <gribble> https://github.com/bitcoin/bitcoin/issues/18095 | Fix crashes and infinite loop in ListWalletDir() by uhliksk ÷ Pull Request #18095 ÷ bitcoin/bitcoin ÷ GitHub 08:04:33 <bitcoin-git> [bitcoin] fanquake closed pull request #18095: Fix crashes and infinite loop in ListWalletDir() (master...master) https://github.com/bitcoin/bitcoin/pull/18095 08:34:50 <bitcoin-git> [bitcoin] MarcoFalke pushed 2 commits to master: https://github.com/bitcoin/bitcoin/compare/c2d8ba6265a4...bcd142e479fa 08:34:50 <bitcoin-git> bitcoin/master c82336c fanquake: Remove references to CreateWalletFromFile 08:34:51 <bitcoin-git> bitcoin/master bcd142e MarcoFalke: Merge #20285: Remove references to CreateWalletFromFile 08:35:09 <bitcoin-git> [bitcoin] MarcoFalke merged pull request #20285: Remove references to CreateWalletFromFile (master...createwalletfromfilenomore) https://github.com/bitcoin/bitcoin/pull/20285 08:36:44 <bitcoin-git> [bitcoin] MarcoFalke pushed 2 commits to master: https://github.com/bitcoin/bitcoin/compare/bcd142e479fa...027e51f71517 08:36:44 <bitcoin-git> bitcoin/master ee11a41 practicalswift: Avoid signed integer overflow when loading a mempool.dat file with a malfo... 08:36:45 <bitcoin-git> bitcoin/master 027e51f MarcoFalke: Merge #20372: Avoid signed integer overflow when loading a mempool.dat fil... 08:37:04 <bitcoin-git> [bitcoin] MarcoFalke merged pull request #20372: Avoid signed integer overflow when loading a mempool.dat file with a malformed time field (master...load-mempool-time-integer-overflow) https://github.com/bitcoin/bitcoin/pull/20372 08:59:48 <bitcoin-git> [bitcoin] MarcoFalke pushed 2 commits to master: https://github.com/bitcoin/bitcoin/compare/027e51f71517...af8ec1d3e576 08:59:48 <bitcoin-git> bitcoin/master 3c77b80 practicalswift: fuzz: Improve coverage for CPartialMerkleTree fuzzing harness 08:59:49 <bitcoin-git> bitcoin/master af8ec1d MarcoFalke: Merge #20375: fuzz: Improve coverage for CPartialMerkleTree fuzzing harnes... 09:00:08 <bitcoin-git> [bitcoin] MarcoFalke merged pull request #20375: fuzz: Improve coverage for CPartialMerkleTree fuzzing harness (master...fuzzers-2020-11-11) https://github.com/bitcoin/bitcoin/pull/20375 09:09:06 <bitcoin-git> [bitcoin] MarcoFalke pushed 2 commits to master: https://github.com/bitcoin/bitcoin/compare/af8ec1d3e576...8a486158cbc3 09:09:07 <bitcoin-git> bitcoin/master 79ef832 practicalswift: tests: Add fuzzing harness for CConnman 09:09:07 <bitcoin-git> bitcoin/master 8a48615 MarcoFalke: Merge #20188: tests: Add fuzzing harness for CConnman 09:09:26 <bitcoin-git> [bitcoin] MarcoFalke merged pull request #20188: tests: Add fuzzing harness for CConnman (master...fuzzers-connman) https://github.com/bitcoin/bitcoin/pull/20188 09:24:18 <bitcoin-git> [gui] hebasto closed pull request #127: Replace QMetaObject::invokeMethod with atomic operations (master...201102-queued) https://github.com/bitcoin-core/gui/pull/127 11:02:43 <jonatack> Sorry to have been reviewing less than usual the past week. After spending time chopping wood on it, #20305 "wallet: introduce fee_rate sat/vB param/option" should be ready for final review for 0.21. 11:02:46 <gribble> https://github.com/bitcoin/bitcoin/issues/20305 | wallet: introduce fee_rate sat/vB param/option by jonatack ÷ Pull Request #20305 ÷ bitcoin/bitcoin ÷ GitHub 11:05:15 <jonatack> Thanks to murch and achow101 for reviewing it so far. 11:24:36 <bitcoin-git> [gui] jonasschnelli merged pull request #120: Fix multiwallet transaction notifications (master...2020-10-fix-transaction-notifications) https://github.com/bitcoin-core/gui/pull/120 11:24:52 <bitcoin-git> [bitcoin] jonasschnelli pushed 4 commits to master: https://github.com/bitcoin/bitcoin/compare/8a486158cbc3...9bd131669729 11:24:53 <bitcoin-git> bitcoin/master 7b3b230 João Barbosa: move-only: Define TransactionNotification before TransactionTablePriv 11:24:54 <bitcoin-git> bitcoin/master 989e579 João Barbosa: qt: Make transaction notification queue wallet specific 11:24:55 <bitcoin-git> bitcoin/master 2414342 João Barbosa: refactor: qt: Use vQueueNotifications.clear() 12:30:51 <vasild> MarcoFalke: one of the test runs on https://github.com/bitcoin/bitcoin/pull/20284 is timing out, I restarted it 2 times, hmm 12:32:03 <MarcoFalke> vasild: Can be ignored 12:32:11 <MarcoFalke> Fix would be to rebase on current master, but we don't want that 12:33:05 <vasild> cirrus is not merging the tip of the PR into latest master before testing, like travis? 12:37:01 <MarcoFalke> not for the config 12:37:08 <vasild> ok 12:37:10 <MarcoFalke> Though the code is merged in the merge_base step 13:23:36 <jonasschnelli> MarcoFalke: as for the devision by 0 in wallet.cpp, ... any idea why cirrus is not triggering the UndefinedBehaviorSanitizer error? 13:24:51 <queip> recommended nomenclature for keys is: SK - secret key, PK - public key, HPK - hash of PK, right? 13:44:47 <MarcoFalke> jonasschnelli: Nope. How is your config different from ./ci/test/00_setup_env_native_asan.sh ? 13:45:44 <MarcoFalke> the ci config is running Ubuntu focal 13:47:05 <jonasschnelli> I guess clang 8 (bbb) vs 10 (cirrus) should not make a difference 13:47:35 <jonasschnelli> bbb doesn't set -DARENA_DEBUG 14:17:04 <shesek> which block validity rules can be validated by spv clients? 14:17:16 <shesek> the ones I have in mind is checking that the previous block hash connects properly, that the nonce matches the target bits, that the target bits match the last difficulty readjustment, that the difficulty readjustments themselves are valid, and the MTP rule. what more am I missing? 14:37:42 <shesek> (I got disconnected and didn't see replies if there were any) 15:18:54 <jonasschnelli> shesek: there where non. :) 15:20:25 <jonasschnelli> shesek: looks like you have a relatively complete list of what measures you can take when not validating the chain 15:20:39 <Kiminuo> shesek, see http://gnusha.org/bitcoin-core-dev/2020-11-12.log 15:20:41 <jonasschnelli> but I recommend to join #bitcoin-dev 15:20:55 <jonasschnelli> this channel is for bitcoin core development 15:26:13 <shesek> jonasschnelli, of course, apologies for going off topic, will take notice next time 15:27:21 <shesek> honestly I kinda knew it, its just that #bitcoin-dev appears pretty much dead, all my scrollback there is join/part/quit messages 15:27:58 <jonasschnelli> shesek: yeah.. maybe also #bitcoin can help 15:28:09 <jonasschnelli> luke-jr have made some thought on that AFAIK. 15:28:15 <jonasschnelli> (on your SPV question). 15:28:21 <pinheadmz> #bitcoin is basically /r/bitcoin 15:28:23 <jonasschnelli> Also look at BIP180 (https://github.com/bitcoin/bips/blob/master/bip-0180.mediawiki) 15:28:31 <pinheadmz> shesek ive gotten a lot of great help from #bitcoin-core-pr-reviews 15:29:39 <bitcoin-git> [bitcoin] practicalswift opened pull request #20377: fuzz: Fill various small fuzzing gaps (master...fuzzers-2020-11-12) https://github.com/bitcoin/bitcoin/pull/20377 15:29:47 <shesek> pinheadmz, is off topic considered okay there when there isn't a meeting going on? 15:29:59 <pinheadmz> yup, get in there 15:30:27 <shesek> jonasschnelli, iirc luke-jr ended up finding some issue with his proposed weight fraud proof mechanism, no? 15:30:49 <jonasschnelli> probably,.. I haven't followed and can't recall 15:39:21 <bitcoin-git> [bitcoin] jonasschnelli opened pull request #20378: fix potential devision by 0 (master...2020/11/fix_devnull_wallet) https://github.com/bitcoin/bitcoin/pull/20378 15:57:54 <warren> jonasschnelli: btw whatever happened to BIP150/151. Looking at BIPs now I don't see a replacement that supersedes them? 15:58:21 <jonasschnelli> warren: BIP324 15:58:26 <jonasschnelli> We are currently altering the AEAD though (make it simpler) 15:58:55 <warren> URL to draft? 15:58:57 <jonasschnelli> https://github.com/bitcoin/bips/pull/1024 15:59:08 <jonasschnelli> discussions also here: https://gist.github.com/jonasschnelli/c530ea8421b8d0e80c51486325587c52#ChaCha20Poly1305Bitcoin_Cipher_Suite 15:59:35 <jonasschnelli> we're getting there... I hope it will not take another 4 years. :) 16:05:01 <warren> thanks just had to point at it as an example 16:06:46 <bitcoin-git> [bitcoin] laanwj pushed 2 commits to master: https://github.com/bitcoin/bitcoin/compare/9bd131669729...0bd4929cd00e 16:06:46 <bitcoin-git> bitcoin/master 38ada89 Vasil Dimov: addrman: ensure old versions don't parse peers.dat 16:06:47 <bitcoin-git> bitcoin/master 0bd4929 Wladimir J. van der Laan: Merge #20284: addrman: ensure old versions don't parse peers.dat 16:07:05 <bitcoin-git> [bitcoin] laanwj merged pull request #20284: addrman: ensure old versions don't parse peers.dat (master...peers_dat_format) https://github.com/bitcoin/bitcoin/pull/20284 16:07:15 <vasild> \o/ 18:32:16 <jnewbery> #proposedmeetingtopic limiting C++17 feature usage (see https://github.com/bitcoin/bitcoin/issues/16684#issuecomment-726214696) 18:41:01 <ja> luke-jr: rhel 8 and fc30 was patched in february according to https://bugs.launchpad.net/ubuntu/+source/glibc/+bug/1864864/comments/2 18:41:43 <ja> luke-jr: writing you here since the thread is already getting really long and i don't wanna add a comment which is answering a question already answered in a posted link 18:46:46 <provoostenator> Looks like Btcd is allergic to sendaddrv2: https://github.com/btcsuite/btcd/issues/1661 (cc roasbeef) 18:48:49 <provoostenator> (I should try an earlier version just to make sure it's not something else...) 18:53:42 <ja> provoostenator: is your testing setup public? it would be fun to try and connect all kinds of different versions with each other and draw a matrix :D 18:54:07 <provoostenator> It connects fine as of a slightly older commit 1d3ec2a1fda7446323786a52da1fd109c01aa6fb 18:54:48 <provoostenator> ja: it's not unfortunately; you'll have to spin up your own btcd and core machine somewhere. 19:00:30 <ja> i see so many booleans, and i am suspicious. for example https://github.com/bitcoin/bitcoin/commit/1d3ec2a1fda7446323786a52da1fd109c01aa6fb 19:00:47 <ja> i understand that it would be too much having a custom type for every single case 19:01:02 <hebasto> meeting? 19:01:04 <achow101> meeting? 19:01:12 <wumpus> #startmeeting 19:01:13 <core-meetingbot> Meeting started Thu Nov 12 19:01:12 2020 UTC. The chair is wumpus. Information about MeetBot at https://bitcoin.jonasschnelli.ch/ircmeetings. 19:01:13 <core-meetingbot> Available commands: action commands idea info link nick 19:01:17 <jnewbery> hi 19:01:19 <amiti> hi 19:01:19 <jonatack> hi 19:01:19 <jonasschnelli> hi 19:01:20 <emzy> hi 19:01:20 <luke-jr> hi 19:01:22 <hebasto> hi 19:01:24 <achow101> hi 19:01:25 <provoostenator> ja: that;s just an arbitary commit, nothing to do with the issue 19:01:26 <provoostenator> hi 19:01:33 <meshcollider> hi 19:01:36 <ja> provoostenator: i would explain if there wasn't a meeting now ;) 19:01:44 <fanquake> hi 19:01:48 <wumpus> #bitcoin -core-dev Meeting: achow101 aj amiti ariard bluematt cfields Chris_Stewart_5 digi_james dongcarl elichai2 emilengler fanquake fjahr gleb gmaxwell gwillen hebasto instagibbs jamesob jb55 jeremyrubin jl2012 jnewbery jonasschnelli jonatack jtimon kallewoof kanzure kvaciral lightlike luke-jr maaku marcofalke meshcollider michagogo moneyball morcos nehan NicolasDorier paveljanik 19:01:50 <wumpus> petertodd phantomcircuit promag provoostenator ryanofsky sdaftuar sipa vasild wumpus 19:02:25 <fjahr> hi 19:02:39 <wumpus> one proposed meeting topic: limiting C++17 feature usage (jnewbery) 19:03:11 <wumpus> I guess the most pressing topic right now is the 0.21.0rc1 release 19:03:41 <wumpus> at least there's only three PRs left on the milestone: https://github.com/bitcoin/bitcoin/pulls?q=is%3Aopen+is%3Apr+milestone%3A0.21.0 19:03:57 <provoostenator> I'd like to get this in the release if possible: https://github.com/bitcoin-core/gui/pull/96 19:04:00 <wumpus> jonasschnelli's last minute fix seems trivial 19:04:10 <jonasschnelli> yes. can be merged I guess 19:04:11 <provoostenator> I reverted the string changes, so it's pretty simple now. 19:04:45 <hebasto> will review it tonight 19:04:58 <wumpus> provoostenator: ok! 19:05:05 <provoostenator> thx 19:05:05 <bitcoin-git> [bitcoin] practicalswift opened pull request #20379: tests: Remove no longer needed UBSan suppression (float divide-by-zero in validation.cpp) (master...remove-ubsan-suppressions) https://github.com/bitcoin/bitcoin/pull/20379 19:05:09 <jonasschnelli> I'll look into it,... but was under the impression that we had code freeze already 19:05:10 <wumpus> yes, it's definitely too late for string changes 19:05:21 <sipa> hi 19:05:36 <provoostenator> The biggest thing that PR does is not recommend encryptino by default 19:05:51 <provoostenator> Which just seems a recipe for tears for brand new users. 19:06:17 <wumpus> I agree with that, encryption is torture for new users 19:06:22 <sipa> which PR are you talking about? 19:06:35 <hebasto> https://github.com/bitcoin-core/gui/pull/96 19:07:04 <hebasto> sipa: ^ 19:08:55 <wumpus> in any case please help review the last remaining PRs for 0.21: https://github.com/bitcoin/bitcoin/pulls?q=is%3Aopen+is%3Apr+milestone%3A0.21.0 and https://github.com/bitcoin-core/gui/milestone/1 19:10:06 <wumpus> #20305 and #18836 are the big ones 19:10:09 <gribble> https://github.com/bitcoin/bitcoin/issues/20305 | wallet: introduce fee_rate sat/vB param/option by jonatack ÷ Pull Request #20305 ÷ bitcoin/bitcoin ÷ GitHub 19:10:12 <gribble> https://github.com/bitcoin/bitcoin/issues/18836 | wallet: upgradewallet fixes and additional tests by achow101 ÷ Pull Request #18836 ÷ bitcoin/bitcoin ÷ GitHub 19:10:47 <luke-jr> 20305 should be noted to break RPC compatibility, but in a way that I and others feel is okay 19:10:57 <luke-jr> if anyone objects, they should speak up 19:11:13 <luke-jr> (see details on PR comments) 19:11:13 <wumpus> versus 0.20? or just versus previous intermediate master releases? 19:11:16 <wumpus> okay 19:11:19 <luke-jr> wumpus: 0.20 19:11:21 <jonatack> (for bumpfee) 19:11:32 <luke-jr> it's complex to explain, but IMO a reasonable concession 19:11:42 <MarcoFalke> That certainly needs release notes, no? 19:11:52 <jonatack> relevant comment by murch on that: "uckily, this is very benign. In the worst case, someone is going to get upped to the minRelayTxFee silently and sends at 1 sat/vB. Since RBF is on by default, they should be able bump when they notice. +1" 19:12:06 <jonatack> https://github.com/bitcoin/bitcoin/pull/20305#discussion_r520231413 19:12:24 <luke-jr> MarcoFalke: agreed 19:12:39 <jonatack> MarcoFalke: yes, definitely, we can edit the wiki. 19:13:31 <luke-jr> most likely trying to use the old interface will just error; worst case, it's fixable 19:14:03 <wumpus> ok 19:14:15 <jonatack> yes, for the bumpfee change, there's a 1e5 times difference in units in the downward direction 19:14:52 <wumpus> yeah downward is the only acceptable direction 19:15:00 <jonatack> also added a * WARNING * in the help 19:15:18 <luke-jr> oh. just thought of a possible danger 19:15:29 <luke-jr> if someone uses the new interface, with an old version 19:16:06 <provoostenator> luke-jr: that's dangerous in general, given some of the other fixes... 19:16:14 <luke-jr> provoostenator: ? 19:16:39 <luke-jr> I wonder if the magnitude would trigger the absurd fee warning 19:16:46 <provoostenator> #16257 19:16:50 <gribble> https://github.com/bitcoin/bitcoin/issues/16257 | [wallet] abort when attempting to fund a transaction above -maxtxfee by Sjors ÷ Pull Request #16257 ÷ bitcoin/bitcoin ÷ GitHub 19:17:01 <sipa> the default absurd fee threshold is 0.1 BTC or so, no? 19:17:22 <jonatack> yes iirc 19:17:35 <jonatack> per the tests i've added in any case 19:17:41 <queip> so it's decided to go with vB? WU was popular last year 19:17:56 <provoostenator> People are still making payments with 0.1 BTC fees FWIW, quite often 19:18:08 <sipa> everyone uses sat/vB, afaik 19:18:24 <wumpus> `if there's any risk of it causing people to overpay fees it's unacceptable risk 19:18:55 <sipa> i think WU is dangerous because it can result in a 4x factor off, which isn't enough to be likely to trigger other warning mechanisms 19:19:00 <jonatack> queip: recent twitter poll showed a 20 to 1 preference for sat/vB over BTC/kB, wasn't an option though 19:19:18 <jonatack> *WU wasn't an option in the poll tho 19:19:21 <luke-jr> sipa: well, nothing is using sat/vB yet.. so it's a factor of 1e5/4 19:19:46 <sipa> luke-jr: almost every fee estimation website, most block explorers, ... do 19:19:54 <wumpus> so is that the case for #20305? 19:19:56 <gribble> https://github.com/bitcoin/bitcoin/issues/20305 | wallet: introduce fee_rate sat/vB param/option by jonatack ÷ Pull Request #20305 ÷ bitcoin/bitcoin ÷ GitHub 19:20:07 <meshcollider> provoostenator: it's the wrong direction, even if they intended to send at 1btc, they'd end up at 1sat/vB 19:20:56 <michaelfolkson> Slight overpayment -> Risk of slight monetary loss. Slight underpayment -> Risk of not being propagated? I guess it depends on how high the original fee was 19:21:02 <luke-jr> minimum realistic bumpfee is probably 2sat/vB? 19:21:17 <meshcollider> I don't think there's ever any risk of overpayment here 19:21:19 <luke-jr> which at BTC/kB is 2 BTC/kB, over the absurd fee cutoff 19:21:26 <jonatack> luke-jr: min incremental fee is 1 sat/vB 19:21:26 <sipa> luke-jr: good point 19:21:35 <wumpus> underpayment is at least better, could always bump again 19:21:52 <luke-jr> so I *think* the magnitude is sufficient to avoid any loss in either direction 19:22:14 <luke-jr> which only leaves the WU question - but I don't think we have time to do another poll 19:22:25 <luke-jr> if people prefer WU and have to divide by 4, it's not the end of the world anywaty 19:22:43 <jonatack> AFAIK sats seem to be far and away what people want 19:22:59 <queip> ok just was asking 19:22:59 <michaelfolkson> +1 19:23:01 <luke-jr> jonatack: well, I assume it'd be sats/WU in that case 19:23:05 <jonatack> found the poll: https://twitter.com/jonatack/status/1318890833131823104?s=20 19:23:06 <emzy> +1 19:24:13 <sipa> i haven't paid attention to the actual solution we're going for now... but would it be easy to add more units later? 19:24:24 <luke-jr> sipa: no 19:24:28 <meshcollider> I don't think it's worth bikeshedding over but I prefer sats/vB honestly 19:24:37 <wumpus> no, I think it's better to settle on one unit and one unit only for RPC 19:24:44 <luke-jr> sipa: this removes all unit choice, and just uses sat/vB 19:24:47 <meshcollider> sipa: we're trying to make it simpler :) 19:25:07 <sipa> luke-jr: how does that not break compatibility (as earlier releases all use BTC/kvB)? 19:25:08 <wumpus> adding a choice of units shouldn't be necesasry, it's a programmatic interface 19:25:18 <jonatack> right, so people don't have to pass in a choice of units with the estimate_mode param 19:25:24 <wumpus> just standardize something ⦠19:25:26 <luke-jr> sipa: the field name is different for everything except bumpfee 19:25:27 <sipa> feel free to tell me to just go read the PR 19:25:36 <luke-jr> sipa: and bumpfee is protected by the magnitude 19:25:56 <luke-jr> sipa: the old interface to bumpfee+fee_rate won't *work*, but it won't do damage either 19:26:26 <sipa> ok 19:26:36 <luke-jr> (other RPCs used "feeRate" instead of "fee_rate") 19:27:07 <luke-jr> IIRC only in options objects, so no positional mess (someone should double-check this) 19:27:26 <jonatack> yes. (in fundrawtxn and walletcreatefundedpsbt only) 19:28:20 <jonatack> feeRate is only an arg, not an option, which we can deprecate as soon as people judge feasible 19:28:31 <jonatack> it's in BTC/kB 19:30:59 <wumpus> next topic? 19:31:07 <jonatack> (fwiw that PR adds a fair number of RPCExamples in send and sendtoaddress to help people use it) 19:31:41 <michaelfolkson> wumpus: yup 19:32:20 <wumpus> #topic limiting C++17 feature usage (jnewbery) 19:32:20 <core-meetingbot> topic: limiting C++17 feature usage (jnewbery) 19:32:38 <jnewbery> hi 19:32:39 <wumpus> link: https://github.com/bitcoin/bitcoin/issues/16684#issuecomment-726214696 19:32:48 <jnewbery> As a reminder, our plan for C++17 is here: https://github.com/bitcoin/bitcoin/issues/16684 19:32:56 <jnewbery> Once we branch v0.21 (imminently), we'll be able to start using c++14 and c++17 features. 19:33:02 <jnewbery> I expect lots of people have their favorite features that they want to start using. 19:33:11 <jnewbery> I was wondering whether we should have a project-wide policy on which new features we should allow, or if there are any we should disallow in the project for now. 19:33:16 <jnewbery> Prompted by https://github.com/bitcoin/bitcoin/issues/16684#issuecomment-726214696, where Cory ran into a bug in glibc when using std::shared_mutex. 19:33:17 <wumpus> so there's a specific feature that triggers a libc bug? 19:33:26 <jnewbery> So std::shared_mutex is one that I think we should avoid, at least until we better understand the bug, but are there others? 19:34:06 <MarcoFalke> std::fs 19:34:08 <wumpus> I'd rather not do that, that for 0.22 full C++17 is allowed, but that is a good reason to disallow that one for now 19:34:18 <hebasto> or define minimum libc with fixed bug? 19:34:19 <sipa> i vaguely remember some issues with trying to use std::filesystem by someone, but i'm not sure if that was because they weren't familiar enough with the build system, or if there were actual problems with it 19:34:40 <MarcoFalke> We can't use std::fs because LTS versions of operating systems don't ship it 19:34:54 <sipa> well what's the alternative? does boost::shared_mutex not have the same problem? 19:34:58 <wumpus> every specific exclusion makes it more difficult for new contributors etc 19:35:11 <MarcoFalke> sipa: We already use boost::shared_mutex 19:35:38 <sipa> MarcoFalke: right, my concern is that boost, when compiled in c++17 mode, will just go "oh the STL provides this, just delegate to that" 19:35:47 <wumpus> people don't generally consult a list of allowed features before contributing 19:35:48 <MarcoFalke> ah good point 19:35:50 <luke-jr> hmm 19:36:14 <wumpus> though things like "use boost::shared_mutex instead of std::shared_mutex" are clear and easy to enforce enough 19:36:21 <wumpus> or the fs one 19:36:29 <wumpus> we have our own fs abstraction anyway 19:36:30 <sipa> it wouldn't be hard to add a linter to outlaw "std::filesistem" and "std::shared_mutex" in the codebase or so 19:36:38 <wumpus> sipa: yes 19:36:41 <wumpus> those two are easy 19:36:51 <MarcoFalke> sipa: The ci would fail if someone tried using std::fs 19:37:00 <sipa> oh, right 19:37:01 <MarcoFalke> (and gitian) 19:37:08 <sipa> my impression is that all these issues are STL features, not languages features 19:37:33 <sipa> and i expect that if you use a sufficient compiler version, all languages features will actually work 19:37:40 <jonatack> luke-jr: you are right, feeRate is an option 19:38:09 <sipa> so perhaps the policy can be "you can use all language features, but initially we'll want to whitelist STL features" ? 19:38:42 <sipa> that's probably too restrictive 19:38:57 <wumpus> no,I think it's better to reject specific things if we have a good reason 19:39:01 <sipa> agree 19:39:03 <wumpus> and just accept standard C++17 otherwise 19:39:14 <sipa> i guess the concern is really about things that compile, but are known to not always work 19:39:17 <sipa> like this shared_mutex thing 19:39:23 <wumpus> right 19:39:27 <ja> jnewbery: should std::shared_mutex be avoided even if it is unknown whether boost::shared_mutex is also broken? 19:39:32 <sipa> and we should just outlaw while necessary... just as with any other compiler/stl bug 19:39:52 <jnewbery> I don't know enough about compilers to know how stable their C++17 stl features are 19:40:11 <wumpus> you would hope it is stable after more then three years 19:40:22 <jnewbery> you would, and yet here we are 19:40:26 <wumpus> of course, compiler and c++ librar ybugs happen sometimes 19:40:32 <wumpus> well yes it's a specific thing 19:40:45 <wumpus> even memcmp had a bug !!! 19:40:49 <jnewbery> we don't know how many other specific things there are 19:40:52 <wumpus> should we reject c89 featurs now 19:40:58 <sipa> haha 19:41:03 <MarcoFalke> Is there a minimal test case for the shared_mutex thing? 19:41:31 <ja> it is still not clear to me which glibc bug it actually is, there are linked two different bugs from the c++17 thread 19:41:45 <ja> but glibc has test cases for the pthread primitives 19:41:46 <wumpus> it's not possible to guarantee that usage of compiler or library features doesn't have bugs, this is just as true for old features as now ones 19:42:23 <wumpus> this is one of the biggest risks in consensus driven systems isn't it 19:42:25 <jnewbery> wumpus: that's doesn't seem like it'd be true. New features are more likely to have bugs 19:42:26 <sipa> boost 1.71 does not seem to use STL's shared_mutex 19:42:42 <wumpus> jnewbery: I'm not convinced about that, code rot is a thing 19:42:44 <jnewbery> *or uncover existing bugs 19:42:59 <wumpus> new features might actually have *more* eyes on them 19:43:22 <luke-jr> sipa: unrelease boost might 19:43:29 <wumpus> this is an abyss without end anyway... we can only calculate in known bugs not potential ones 19:44:05 <luke-jr> if every distro has a fix, maybe just a sanity check is in order 19:44:12 <MarcoFalke> A future release of boost might decay boost::shared_mutex into std::shared_mutex 19:44:12 <jnewbery> My general point is that we shouldn't rush to use new features unless there's very clear benefit 19:44:24 <wumpus> MarcoFalke: yep 19:44:27 <luke-jr> otoh, isn't it just a deadlock worst case? 19:44:49 <jnewbery> and that if we do that, then we should codify what features can be used at the project level 19:44:53 <luke-jr> I think deadlock is fairly harmless for user-built binaries that simply require an updated OS to fix 19:45:09 <wumpus> no, I don't think we should make a long list of allowed c++17 features 19:45:10 <ja> luke-jr: if you use bitcoin to back a lightning node, a deadlock could be pretty bad, no? 19:45:16 <wumpus> only exclude ones that we know to cause problems 19:45:32 <luke-jr> ja: hmm, maybe - but aren't there supposed to be watchers? 19:46:05 <wumpus> we have already made the decision to use c++17, maybe we should be more careful in consensus code, but that's *always* the case for any change 19:46:06 <ja> luke-jr: i don't know how many people are running watchers. it is irrelevant whether there is supposed to be 19:46:43 <luke-jr> point is, that would be a scenario with two user errors 19:47:09 <michaelfolkson> Watchtowers only needed if you aren't online 100 percent of time (or for additional protection) 19:47:26 <wumpus> and sure, we should take the same care as we did for c++11, don't change things for the sake of changing them 19:47:31 <ja> michaelfolkson: if you bitcoin node is deadlocked on a shared_mutex, are you online? 19:48:02 <michaelfolkson> I'd guess not ja :) 19:48:05 <wumpus> that should also be standard policy for the project already, no needless refactors 19:48:57 <michaelfolkson> But how long would you be deadlocked for? 19:49:08 <ja> 2 weeks, until you come back from vacation 19:49:09 <jnewbery> I think only introducing new features as they're needed is the more cautious approach 19:49:12 <luke-jr> michaelfolkson: indefinitely 19:49:18 <sipa> michaelfolkson: the definition of a deadlock implies it's forever 19:49:31 <michaelfolkson> Ok. That is a problem 19:49:38 <MarcoFalke> jnewbery: I don't think C++17 is "needed". We could stay with C++11 forever 19:49:41 <wumpus> in new code I think full c++17 should be allowed (apart from what we have labaled as dangerous features) 19:49:44 <luke-jr> but the same goes for any DoS vuln 19:49:48 <luke-jr> which seem fairly common 19:49:49 <wumpus> yes, we could stay in c++11 forever 19:50:24 <wumpus> but we've already decided not to 19:50:42 <jnewbery> It would be nice if we could have some nuance in these discussions instead of talking about C89 and staying on C++11 forever 19:50:49 <wumpus> I don't think it would make the project any safer 19:51:12 <hebasto> we coudn't stay on C++11 due to new Qt versions 19:51:31 <luke-jr> hebasto: Qt dropped C++11 support? :o 19:51:32 <wumpus> just make an exception for the gui code... 19:51:40 <sipa> waot 19:51:49 <MarcoFalke> Use C++11 code, but compile it with -std=c++17 19:51:49 <sipa> this shared_lock thing sounds like a problem in pthread? 19:51:58 <sipa> which means it would also affect boost? 19:52:13 <wumpus> boost definitely uses pthread (but maybe in a different way?) 19:52:30 <hebasto> luke-jr: #19716 19:52:32 <gribble> https://github.com/bitcoin/bitcoin/issues/19716 | build: Qt 5.15.x by fanquake ÷ Pull Request #19716 ÷ bitcoin/bitcoin ÷ GitHub 19:52:46 <michaelfolkson> jnewbery: I think the nuance goes when wumpus says new contributors won't consult guides. But agreed I'd guess nuance is the better way to go 19:52:50 <wumpus> if it's a general pthread issue then we're already affected and c++17 is unimportant to this 19:52:56 <sipa> indeed 19:52:58 <jonatack> At the end of the day, these things are going to come down to code review and incremental adjustments and guidelines as needed. 19:53:03 <sipa> boost has its own shared_lock implementation 19:53:10 <luke-jr> hebasto: that looks like the opposite 19:53:12 <sipa> and doesn't use pthread's rwlock interface 19:53:18 <wumpus> great 19:53:25 <sipa> (in 1.71) 19:53:37 <wumpus> michaelfolkson: I don't think we can front-run any compiler or c++ library issues 19:54:21 <wumpus> avoiding c++17 features would give a false sense of security imo, it's not like it's brand new 19:54:31 <luke-jr> I wonder if we ought to add a CI using roconnor's memcmp bug detection 19:54:36 <wumpus> but that's just my opinion 19:55:03 <luke-jr> seeing as GCC/distros appear to have a disinterest in actually fixing it 19:55:11 <fanquake> luke-jr: opposite of what? Qt started using C++14 features in its code, and essentially âÂÂforgotâ that it was still meant to support c++11. An issue was opened but they never did anything to fix the issue. 19:55:57 <luke-jr> fanquake: I don't see that mentioning in the linked issue? 19:56:17 <wumpus> I'm not sure how more nuanced you want this, I don't think it's useful to evaluate every single c++17 feature in the meeting at least 19:56:45 <wumpus> as if we can judge how much risk the compiler or library change is anyway 19:56:49 <luke-jr> fanquake: you can't build Qt with C++14 and then link from C++11 code? 19:57:39 <wumpus> could you ever have predicted this? 19:57:53 <wumpus> did std::shared_mutex sound dangerous to you than boost::shared_mutex? 19:58:15 <wumpus> still, here we are 19:58:20 <jnewbery> changing to something new is always dangerous 19:58:31 <fanquake> luke-jr: I mentioned the details and linked to upstream in #19305 19:58:34 <gribble> https://github.com/bitcoin/bitcoin/issues/19305 | doc: add C++17 release note for 0.21.0 by fanquake ÷ Pull Request #19305 ÷ bitcoin/bitcoin ÷ GitHub 19:58:39 <wumpus> okay, so not change to anything new anymore then? 19:58:42 <sipa> jnewbery: boost has also had its fair share of bugs though 19:58:54 <wumpus> would this *specifically* have been risky to you? 19:59:06 <sipa> and a priori it seems a reasonable assumption that things that make it into the C++ standard have matured enough that they're less risky 19:59:07 <wumpus> if not, it would be a blanket reject c++17 features 19:59:15 <wumpus> yes 19:59:35 <wumpus> and yes, boost versions have also broken things sometimes 19:59:58 <wumpus> not upgrading boost is dangerous too, though 20:00:55 <MarcoFalke> With other project upgrading, at some point boost::feature is going to be less tested than std::feature. Code changes inside boost, which we can't anticipate are going to eventually break the feautre 20:01:24 <jnewbery> I don't see any strong arguments against being cautious about new features, but I'm not getting the impression that I've convinced anyone 20:01:27 <wumpus> I don't think any general principple can be derived from this 20:01:46 <wumpus> yes, being cautious is always good 20:01:49 <wumpus> I hope we already are cautious? 20:02:01 <sipa> jnewbery: i think we should always be caution about new features, but i don't think this is very specific to new language/stl features 20:02:04 <wumpus> do you have any specific suggestion? 20:02:43 <sipa> and we do have a review and QA cycle, which are part of the process 20:02:43 <queip> as it was mentioned, compiling for old mac os x will break, also related to SDK versions afair. In context of Gitian for example 20:03:00 <ja> the decision should ideally be derived from usage patterns, not from how new the feature is. a 10 year old language feature that nobody uses can't be relied on. the age of the feature is a proxy for how widely used it is, which is a proxy for how buggy it is 20:03:10 <queip> that is, c++!7 forces such bump of minimal macosx version 20:03:19 <jnewbery> I didn't think sipa's suggestion of having a list of allowed features was bad. That list would grow over time 20:03:22 <wumpus> queip: yes, I think that wa calculated in 20:03:29 <wumpus> I don't think that's a good idea 20:03:58 <wumpus> it's super confusing to new contributors for example 20:04:03 <michaelfolkson> Because of new contributors and because of not knowing what should go on that list wumpus? 20:04:07 <sipa> i think i agree with wumpus now 20:04:08 <wumpus> yes 20:04:21 <luke-jr> queip: we typically only guarantee support for the most recent stable version of an OS 20:04:28 <luke-jr> though Windows is apparently an exception 20:04:35 <sipa> what constitutes a "feature" even? is a new member function that was added to std::vector a "feature" ? 20:04:36 <wumpus> if we really dislike certain features then we should disallow them, but I don't think it makes sense to partially allow a standard 20:04:43 <wumpus> yes 20:04:50 <wumpus> do we hae to whitelist every function? every class? 20:05:05 <MarcoFalke> Generally, our insurance against build system bugs are tests 20:05:16 <wumpus> do you even want to maintain this list? 20:05:46 <sipa> maybe a good question to ask ourselves: if we had started using std::shared_mutex, would we have caught this issue before release? 20:06:33 <wumpus> only if we had a test reproducing the issue predictably I think 20:06:56 <MarcoFalke> we should add one 20:06:58 <jnewbery> wumpus: I don't think we should be setting our project standards based on what features new contributors might want to use. We have project standards precisely so all contributors write code in a common way 20:06:59 <sipa> yeah, it'd probably depend on how actively shared_mutex was used (and with how much contention...) 20:07:06 <wumpus> if it causes a random hang in travis, well, peple would think it's just another infrastructure issue 20:07:28 <wumpus> jnewbery: okay 20:07:31 <sipa> jnewbery: that conceptually makes sense, but what specifically is your suggestion? 20:07:42 <wumpus> jnewbery: feel free to make a list 20:07:45 <wumpus> jnewbery: and PR it 20:07:56 <wumpus> jnewbery: I'm not conceptually against it I just do not want to maintain it 20:07:59 <MarcoFalke> time, btw 20:08:00 <michaelfolkson> The new contributor argument I don't think is particularly convincing. But the problems of constructing a list is more convincing to me 20:08:00 <luke-jr> "Travis failed, can someone restart it for me?" 20:08:10 <queip> apparently not popular opinion, but a white list of allowed free functions, and classes, would make it easier to guard against someone using obscure function that happens to be buggy. Though also a review can anyway guard against it - "why not use the more common solution to this problem". Either way read list of, probably 20 recommended classes and functions, is not that hard for new developer 20:08:24 <wumpus> queip: again, feel free to maintian such a thing 20:08:38 <jnewbery> queip: that seems easier to me than arguing on each PR what is and isn't acceptable 20:08:59 <wumpus> I'm done with this (both the meeting and the discussion) 20:09:00 <wumpus> #endmeeting