< bitcoin-git>
bitcoin/master 8bba91b John Newbery: [wallet] Fix whitespace in CWallet::CommitTransaction()
< bitcoin-git>
bitcoin/master b6f486a John Newbery: [wallet] Add doxygen comment to CWallet::CommitTransaction()
< bitcoin-git>
bitcoin/master d1734f9 John Newbery: [wallet] Remove return value from CommitTransaction()
< bitcoin-git>
[bitcoin] laanwj merged pull request #17154: wallet: Remove return value from CommitTransaction (master...2019-04-CommitTransaction) https://github.com/bitcoin/bitcoin/pull/17154
< bitcoin-git>
[bitcoin] promag opened pull request #17237: wallet: LearnRelatedScripts only if KeepDestination (master...2019-10-wallet-reservedestination) https://github.com/bitcoin/bitcoin/pull/17237
< fanquake>
Interestingly, there are at least two members of the Rust Core team in the same group, and it's looks like we are sharing some of the same GitHub grievances as them.
< wumpus>
meeting time?
< MarcoFalke>
yeah
< wumpus>
#startmeeting
< lightningbot>
Meeting started Thu Oct 24 19:00:51 2019 UTC. The chair is wumpus. Information about MeetBot at http://wiki.debian.org/MeetBot.
< wumpus>
oh, well, mac functional tests are gone now, you shouldn't have to rebase anymore for that
< instagibbs>
hi
< wumpus>
I think we have plenty of time, no topics have been suggested for today; though I think we need to discuss 0.19.0rc2 as well
< jtimon>
can we add #17037 to chasing concept ack?
< gribble>
https://github.com/bitcoin/bitcoin/issues/17037 | Testschains: Many regtests with different genesis and default datadir by jtimon . Pull Request #17037 . bitcoin/bitcoin . GitHub
< wumpus>
#topic BIP157 (provoostenator)
< provoostenator>
I found some issues while testing against Lnd / Btcd
< provoostenator>
cc roasbeef
< wumpus>
jtimon: added
< digi_james>
hi
< jtimon>
thanks
< instagibbs>
provoostenator, testing what against, 0.19?
< provoostenator>
btcd uses a max getcfilters of 1000
< provoostenator>
Where the BIP uses 100
< jeremyrubin>
suggested topic: mempool limits
< provoostenator>
So the #16442 will disconnect from those
< provoostenator>
I believe the rationale for 100 was to get those messages to about 2 MB
< provoostenator>
Bigger means fewer round dtrips for mobile.
< provoostenator>
I don't know if there's a downside to bigger...
< provoostenator>
We don't send these things unsollicited
< MarcoFalke>
That sounds like a bug in either btcd or the bip? Maybe the mailing list is a better place to discuss?
< provoostenator>
Converesy, we don't have a rate limiter for this in the PR. Lnd, when "misconfigured" will happily fetch gigabytes per minute...
< provoostenator>
Yeah, mailinglist makes sense regardless, but was hoping to find opinions here first.
< sipa>
provoostenator: how does the misconfiguration manifest?
< sipa>
is it fetching the same block over and over?
< provoostenator>
sipa: when it checks lightning channel gossip, it refetches old filters all the time
< provoostenator>
That's an Lnd bug imo
< provoostenator>
But someone can do this intentionally too
< sipa>
of course
< provoostenator>
Do we have any rate limiting on block fetching and such?
< sipa>
not afaik
< provoostenator>
Ok, I guess in that case there's not much precedent to add it for filters.
< wumpus>
no, there's no rate limiting on block fetching
< wumpus>
it's only limited by the I/O speeds, disk and network
< MarcoFalke>
or by -maxuploadtarget
< wumpus>
the extra DoS vector with bloom filters is that it allowed to do a DoS on the disk without actually having to receive the data over the network, but, it's easy to saturate bandwidth
< wumpus>
yes, there's that
< jnewbery>
Can we add #15934 to high priority? It's blocking three other PRs which add quite nice functionality (#15935, #15936, #15937)
< sipa>
BIP157 doesn't have the same problem as the I/O required is proportional to what is sent over the network
< wumpus>
right
< wumpus>
jnewbery: sure, though I think with 10 blockers in high prio we're kind of pushing it
< jamesob>
+1 on 15934
< jamesob>
(but agree the list is getting long)
< provoostenator>
Ok, so any thoughts on the maximum size of filter messages we send (ignoring the BIP)?
< jnewbery>
wumpus: how about if I promise to review some of the other ones? :)
< instagibbs>
wumpus, people have different interests in subtopics, i dont think "long" hurts more than too many type collsions
< wumpus>
jnewbery: great!
< wumpus>
instagibbs: 10 is fine
< instagibbs>
:)
< jeremyrubin>
I've been making fine progress on the things that depend on #16766, so am OK with either removing from high priority while it gets more review or else I think it's basically mergeable now.
< jeremyrubin>
I guess it's fine for post meeting; but I wanted to get examples/edge cases people know of bad mempool behaviors that justify the limits currently and collect any tests people have written to benchmark this
< jeremyrubin>
Because I think there's a bit of a documentation gap for why certain limits exist and the intended protection (or, if additional protections conferred became known post-hoc)
< instagibbs>
Unfortunately lots of mempool design is communal knowledge spread among like 5 people.
< bitcoin-git>
[bitcoin] amitiuttarwar opened pull request #17243: tools: add PoissonNextSend method that returns mockable time (master...1910-mockable-poisson) https://github.com/bitcoin/bitcoin/pull/17243
< jeremyrubin>
instagibbs: this is one way to fix it ;)
< wumpus>
jeremyrubin: oh sorry I forgot your topic
< instagibbs>
I'm concept ACKing your call. I've previously asked for a "philosophy of design" type document, sdaftuar wrote something(now I cannot find the link, oops)
< wumpus>
jeremyrubin: maybe propose it for next week
< instagibbs>
that seems to be more block related, but transaction gossiping would be a good one
< wumpus>
elichai2: it looks like -fno-stack-check disables stack alignment check, not security checks like the stack protector
< wumpus>
hm or maybe not, I'm not sure
< elichai2>
well theoretically x86 is fine with unaligned reads/writes. altough I have no idea if it's even related to this :D
< wumpus>
I think there's an exception for some instructions like AVX2
< sipa>
movdqa requires aligned arguments
< elichai2>
<elichai2> the bug is in AVX assembly *produced by the compiler* (i.e. secp has no avx)
< elichai2>
wumpus: oh. you meant about read alignments. sorry
< wumpus>
thinking of it, it might generate that AVX code to check the stack cookie
< wumpus>
assuming the stack pointer is aligned
< sdaftuar>
instagibbs: i also have some high level slides lying around somewhere that explain mempool design if you're interested
< instagibbs>
sure
< jonatack>
jeremyrubin: Thanks, it seems like a good idea to assemble this information, building on jnewbery's document instagibbs linked to and sdaftuar's slides. I'd be interested.
< elichai2>
I just tried to convert `DecodeDumpTime` from using boost to `std::get_time` and chrono. and got into timezone rabbit hole
< elichai2>
i'm joking, trying to work on a weirder way, just really hoped I can do it using libstd
< luke-jr>
[19:16:40] <jnewbery> Can we add #15934 to high priority? It's blocking three other PRs which add quite nice functionality (#15935, #15936, #15937) <-- more like conflciting with..
< luke-jr>
oh, those listed are the poorly rewritten ones :/
< luke-jr>
#11082 should go in instead ;)
< gribble>
https://github.com/bitcoin/bitcoin/issues/11082 | Add new bitcoin_rw.conf file that is used for settings modified by this software itself by luke-jr . Pull Request #11082 . bitcoin/bitcoin . GitHub
< jonatack>
luke-jr: your PR came up in the review club discussion yesterday (https://bitcoincore.reviews/15934.html). Seems PRs 11082 and 15934 ought to be reviewed concurrently then.
< luke-jr>
jonatack: well, 11082 vs 15935 really; 15934 is probably rebasable
< luke-jr>
11082 has been in production use for years at this point
< bitcoin-git>
bitcoin/master 168b781 Anthony Towns: Continue relaying transactions after they expire from mapRelay
< bitcoin-git>
bitcoin/master fce7c75 MarcoFalke: Merge #16851: Continue relaying transactions after they expire from mapRel...
< bitcoin-git>
[bitcoin] MarcoFalke merged pull request #16851: Continue relaying transactions after they expire from mapRelay (master...201909-relayparents) https://github.com/bitcoin/bitcoin/pull/16851
< MarcoFalke>
elichai2: no
< elichai2>
MarcoFalke: so if I want to add a unit test to a static function I must make it non-static?
< MarcoFalke>
yes
< MarcoFalke>
otherwise it wouldn't be properly linked into the test_bitcoin, I think
< * luke-jr>
wonders if he should submit PRs for everything blocked on #11082..
< gribble>
https://github.com/bitcoin/bitcoin/issues/11082 | Add new bitcoin_rw.conf file that is used for settings modified by this software itself by luke-jr . Pull Request #11082 . bitcoin/bitcoin . GitHub
< elichai2>
MarcoFalke: well one ugly way would be to `#include` the cpp file heh
< sipa>
you can test a static function if it's defined in the same compilation unit as the test
< sipa>
anything else would grossly violate C++
< sipa>
(that's the definition of static: accessible within the same compilation unit)
< MarcoFalke>
elichai2: Oh right. Forgot about that, but I'd rather not do that.
< elichai2>
sipa: so if i'm testing a static function from rpcdump.cpp in wallet_tests.cpp I must make it non static :/
< sipa>
yes
< sipa>
or put in a .h file, and make it static inline; that also works :p
< sipa>
or just static;
< elichai2>
MarcoFalke: yeah, that's seriously ugly and would give us trouble in the future on using the wrong types / redeclaring the same functions etc.
< luke-jr>
sipa: I think we've been moving away from that?
< elichai2>
I guess no static it is
< * luke-jr>
remembers when GetMinFee was in .h
< nothingmuch>
typically what is the rationale behind making things static (not in general, in core)?
< luke-jr>
so the compiler cna inline it?
< elichai2>
nothingmuch: I would guess if it's a small function used only in that compilation unit that you want to be easily inlined
< sipa>
s/easily//
< MarcoFalke>
elichai2: Which function is it?
< elichai2>
core doesn't get special treatment from gcc yet :P
< elichai2>
MarcoFalke: DecodeDumpTime
< elichai2>
wrote a test case that asserts that the old and new ones returns the same value
< sipa>
something in another compilation unit cannot be inlined (except through LTO)
< nothingmuch>
thanks, i thought maybe it was something more... in that case wouldn't a non static wrapper also do the job?
< MarcoFalke>
Yeah, that should be exposed in the header I guess
< elichai2>
sipa: yeah but in the same compilation unit people tend to think(hope?) that static has more chance to be inlined
< MarcoFalke>
for rpc code such inline performance doesn't matter
< MarcoFalke>
also, the rpc*.cpp files should not contain any logic except parsing UniValue and then calling an util function
< elichai2>
MarcoFalke: so to which header do you think it should move? rpcwallet.h?
< MarcoFalke>
That's the easiest for now
< MarcoFalke>
If you feel fancy, you could move it to something like ./src/wallet/util