< wumpus>
the board running my IRC client died yesterday, so I was gone for a while and have no logs. Currently using a temporary workaround hosting elsewhere. Did I miss anything?
< aj>
wumpus: bluematt pinged you and fanquake mentioned #13093 at you i think
< kallewoof>
wumpus: Is it possible to add #12634 to the high priority list? It would be very helpful in trimming down the output group (bundle address reuse UTXO) PR.
< harding>
jonasschnelli, wumpus: for the website, the GitHub is setup so that I can't merge my own PRs unless they have an "Approve" review from someone else with commit access. The following PR numbers there have ACKs from known contributors and so are just waiting for someone besides me with commit access to Approve them: 546, 547, 550, 551, 552, and 553.
< fanquake>
kallewoof I've added it
< kallewoof>
fanquake: Thanks
< murrayn>
any chance
< murrayn>
of getting some feedback on 12881?
< promag>
murrayn: i think it's mergeable
< murrayn>
yes i saw your utACK, thanks
< promag>
murrayn: np, you can pay me by reviewing #13193 x)
< wumpus>
kallewoof: sure - I think it's usually most helpful to discuss the high priority list during the meeting, but we can do it in-between if you're in a hurry. Oh I see someone already added it anyway :)
< wumpus>
harding: yes I'm extremely behind on website review, will take a look!
< kallewoof>
wumpus: Yeah, I just tend to not be awake 4 am. I am trying to pull it off though. Maybe this week...
< wumpus>
kallewoof: yes that's a crazy time, if you can't make it, no problem
< kallewoof>
wumpus: It should be doable, but I tend to wake up late. 4 am is usually a few hours after I fall asleep :)
< wumpus>
murrayn: I haven't looked at it since my comment about locale-specific functions, will re-review
< wumpus>
murrayn: what was your motivation for optimizing bech32::Decode? is this on any critical path?
< bitcoin-git>
[bitcoin] ken2812221 opened pull request #13236: break circular dependency: random -> util -> random (master...random_util) https://github.com/bitcoin/bitcoin/pull/13236
< wumpus>
harding: looks like the problem is also that people are using ACKs, not the approval system
< wumpus>
should probably become routine in that repository that people who utACK/ACK also 'approve' the PR
< murrayn>
wumpus, the original motivation was just something that caught my eye as an optimization.
< wumpus>
murrayn: right, I see your point, but at some point I wonder about the cognitive load of reviewing that a change is correct versus the reason to make it
< murrayn>
wumpus, yes i see your point re: the cognitive load. in this case the change and its effect is very localized.
< wumpus>
there's certainly a place for micro-optimizations, but I'm not sure this is it
< wumpus>
I agree it's a very localized change, sure
< harding>
wumpus: true about ACKs versus Approvals, although I think most of the ACKs on those PRs are from people without commit access, so GitHub doesn't wouldn't count their Approves towards unlocking mergability. That's a bit unfortunate to my mind, as I think it somewhat discourages people who don't alreday have commit access from reviewing.
< wumpus>
murrayn: anyhow I'm going to ACK it - but please next time if you optimize, try to find something that is on the critical path (e.g. validation) and can clearly demonstrate improvement
< wumpus>
murrayn: and this makes the code somewhat easier to read, that's good
< wumpus>
harding: oh, I didn't know only approvals of repo members counted. Maybe we should remove this github-side requirement and leave it up to the committer to count the ACKs?
< murrayn>
wumpus, ACK. as sipa pointed out, it's not an important function to optimize, so it does come down to readability.
< wumpus>
murrayn: apart from validation there's some other things that would benefit from optimization: for example some GUI things with regard to handling lots of transactions/addresses, network code, RPC/web server latency
< wumpus>
handling of large wallets
< murrayn>
wumpus, i'm working on it, getting my feet wet :-)
< harding>
wumpus: I'm in favor of that. I initially thought it was a security requirement, and it makes sense to me that you'd want to prevent any person from unilaterally pushing to the master branch for a live-deployed website. But then I realzed how easy it would be to circumvent by creating a secondary account to open PRs from, so I'd prefer to indeed just do the usual 'count PRs from known contributors and ensure there are enough
< bitcoin-git>
bitcoin/master 60f61f9 murrayn: Tighten up bech32::Decode(); add tests.
< bitcoin-git>
bitcoin/master 1d4662f Wladimir J. van der Laan: Merge #12881: Minor optimizations to bech32::Decode(); add tests....
< bitcoin-git>
[bitcoin] laanwj closed pull request #12881: Minor optimizations to bech32::Decode(); add tests. (master...bech32_decode) https://github.com/bitcoin/bitcoin/pull/12881
< wumpus>
harding: yes from a security point of view it makes some sense, espeically if only repo members are counted
< wumpus>
harding: so are you sure only people with *write access* count, or everyone who is part of the repository team, also read-only members? if the latter we could just add everyone from the bitcoincore organization in it
< harding>
wumpus: good question. The GitHub UI says, "At least 1 approving review is required by reviewers with write access." Looking deeper at the documentation, it seems to confirm that, and I think that's what I observed for one of MarcoFalke's Approves (that the UI still said I needed a review by someone with write access).
< harding>
wumpus: in any case, thanks for all your reviews this morning!
< wumpus>
harding: np, feel free to poke me when you need review for the website again
< provoostenator>
Two PRs with baby steps towards making RBF easier to use: #12096, #12818
< gribble>
https://github.com/bitcoin/bitcoin/issues/12096 | [rpc] [wallet] Allow specifying the output index when using bumpfee by kallewoof · Pull Request #12096 · bitcoin/bitcoin · GitHub
< provoostenator>
I have two nodes on master and a v0.16.0 which all seem to refuse to relay 1 sat / byte transactions, despite "minrelaytxfee": 0.00010000 ...
< provoostenator>
The nodes on master don't throw any errors when you use the GUI or sendrawtransaction. The 0.16.0 node however compalins that the minimum relay fee isn't met.
< sipa>
provoostenator: 0.0001 is 10sat/byte, no?
< sipa>
so not relaying 1sat/byte seems expected
< provoostenator>
I set maxmempool to something really large, but on the 0.16 node I forgot to remove minrelaytxfee, which indeed set it to 10.
< provoostenator>
The other two nodes have 0.000010000, but once I fixed the 0.16.0 node transactions get relayed as expected, so nvm...
< provoostenator>
Different issue: I'm comparing #12404 with master on a node with 1 GB ram. I didn't confiugre dbcache, yet it OOM'd at cache=568 MiB (on master as well as my PR)
< provoostenator>
Is there room wiggle room above the default 450 MB or could this be a bug?
< sipa>
there's a couple hundred MB in other things, yes
< provoostenator>
Wouldn't it be better for dbache to be inclusive of these "other things"? So it would flush when the total hits 450, rather than some unpredicatable(?) point?
< provoostenator>
It flushed at 716 MB (April 2013), so I'll try dbcache=200 on the 1 GB machine which leave 100 MB margin of error. This is useful too: https://gist.github.com/laanwj/efe29c7661ce9b6620a7
< provoostenator>
(that last number was on a machine with more RAM, I'll keep it running to see where the other peaks are)
< sipa>
provoostenator: it's very hard to account for all memory
< sipa>
we could include a few easily-accountable things like block headers
< sipa>
and i'm generally in favor of ways of making total memory more predictable
< provoostenator>
But the log entries say "cache=223.1MiB" so at least that number is known and could be kept below dbcache, right?
< sipa>
yes it should be
< provoostenator>
(except during the flush itself)
< sipa>
below dbcache+mempoolsize
< provoostenator>
Ah, so setting maxmempool would also make it more predicatable?
< sipa>
mempoolsize + dbcache should always be below -maxmempool + -dbcache
< provoostenator>
Isn't the mempool empty during IBD?
< sipa>
yes, that's the point
< sipa>
dbcache can "borrow" the unused mempool memory
< sipa>
because during IBD we have mempool memory to spare, and more dbcache helps a lot
< provoostenator>
That makes sense. Would be useful to mention that in the -dbcache command documentation.
< provoostenator>
Though the "reducing bitcoind memory usage" doc does mention this.
< sipa>
sounds good
< sipa>
I wasn't aware it wasn't mentioned
< BlueMatt>
sipa (or whoever): you wanna re-ack-and-merge #13023? then we can tag 16.1rc1 =D
< BlueMatt>
(the fd things I dont think make sense to wait for, they haven't made much progress that I see, unless cfields wants to object)
< skeees>
=)
< bitcoin-git>
[bitcoin] Empact opened pull request #13239: [moveonly] Fix CConnman template methods to be fully-defined in net.h (master...net-template-methods) https://github.com/bitcoin/bitcoin/pull/13239
< cfields>
BlueMatt: whoops, never ended up getting back to that
< cfields>
I'm not too worried though
< BlueMatt>
cfields: yea, I'm not either
< bitcoin-git>
[bitcoin] Empact opened pull request #13241: scripted-diff: Avoid temporary copies when looping over std::map (master...pair-const-key) https://github.com/bitcoin/bitcoin/pull/13241