< bitcoin-git> [bitcoin] fanquake pushed 2 commits to master: https://github.com/bitcoin/bitcoin/compare/f4cfa6d01900...007e15dcd7f8
< bitcoin-git> bitcoin/master ef3d4ce fanquake: build: call AC_PATH_TOOL for dsymutil in macOS cross-compile
< bitcoin-git> bitcoin/master 007e15d fanquake: Merge #19565: build: call AC_PATH_TOOL for dsymutil in macOS cross-compile...
< bitcoin-git> [bitcoin] fanquake merged pull request #19565: build: call AC_PATH_TOOL for dsymutil in macOS cross-compile (master...cant_find_dsymutil) https://github.com/bitcoin/bitcoin/pull/19565
< vasild> Is there a python function in test/functional that can be used to serialize varint?
< vasild> There is ser_compact_size() test/functional/test_framework/messages.py, but I can't seem to find one for varint
< luke-jr> this error makes no sense and I can't reproduce it :/ https://travis-ci.org/github/bitcoin/bitcoin/jobs/711362669#L3131
< vasild> src/wallet/walletdb.h:44:using WalletDatabase = BerkeleyDatabase;
< vasild> luke-jr: maybe it picked some "strange" berkeley db version that does not have "env" and "GetFileId" members?
< vasild> -I/home/travis/build/bitcoin/bitcoin/db4/include
< luke-jr> vasild: that's not a bdb class
< vasild> hm, right, it is defined in src/wallet/bdb.h:101:class BerkeleyDatabase
< vasild> luke-jr: I can't reproduce it locally either, but it failed to compile for me for another reason: https://bpa.st/PUHA
< luke-jr> yeah, I was wondering why I wasn't getting that one..
< vasild> clang 11 here
< luke-jr> hmm, does GCC lack it entirely? :/
< vasild> g++10 -Wthread-safety-analysis
< vasild> g++10: error: unrecognized command-line option '-Wthread-safety-analysis'
< luke-jr> apparently it was removed at some point :/
< bitcoin-git> [bitcoin] hebasto closed pull request #19567: p2p, refactor: Do not over-reserve vAddr capacity (master...200722-addr) https://github.com/bitcoin/bitcoin/pull/19567
< achow101> luke-jr: you need to rebase
< achow101> An actual WalletDatabase virtual class was added. It doesn't have an env member
< achow101> Do you really need the bdb unique file id? Sqlite doesn't have a similar id so it's not guaranteed that file IDs will exist for all WalletDatabase classes
< wumpus> yanmaani: it should be preferred to use named parameters with RPC where possible as they prevent some kinds of bugs, positional arguments are mainly still accepted for historical compatiblity (no, they're not going away any time soon if it's up to me)
< achow101> Personally I still prefer positional because I can never spell the named args correctly the first time
< wumpus> right for use in a program is different than using it manually
< jonatack> +1. for manual use, I mostly use positional.
< wumpus> in principle bitcoin-cli could add in the names before sending it to the server and emulate the same interface
< jonatack> heh, just realised i've made 4 failed CLI PRs in a row, and 16439 by aj has been open for exactly a year now with only one ACK (from me)... CLI PRs seem unsexy :D
< bitcoin-git> [bitcoin] troygiorshev opened pull request #19580: Remove message_count (master...2020-07-remove-message_count) https://github.com/bitcoin/bitcoin/pull/19580
< instagibbs> jonatack, clearly the -cli is perfect then
< elichai2> harding: Thanks! russell's tool was exactly what I was looking for :)
< jonatack> instagibbs: yup seems so
< jnewbery> wumpus: I think #19472 is RFM. It has 4 ACKs
< gribble> https://github.com/bitcoin/bitcoin/issues/19472 | [net processing] Reduce cs_main scope in MaybeDiscourageAndDisconnect() by jnewbery · Pull Request #19472 · bitcoin/bitcoin · GitHub
< wumpus> jnewbery: thanks
< yanmaani> possible to subscribe to bitcoin-dev list without using the browser? captcha doesn't work on tor
< bitcoin-git> [bitcoin] laanwj pushed 5 commits to master: https://github.com/bitcoin/bitcoin/compare/007e15dcd7f8...40a04814d130
< bitcoin-git> bitcoin/master 1a1c23f John Newbery: [net processing] Change cs_main TRY_LOCK to LOCK in SendMessages()
< bitcoin-git> bitcoin/master a1d5a42 John Newbery: [net processing] Fix bad indentation in SendMessages()
< bitcoin-git> bitcoin/master a49781e John Newbery: [net processing] Only call MaybeDiscourageAndDisconnect from SendMessages
< bitcoin-git> [bitcoin] laanwj merged pull request #19472: [net processing] Reduce cs_main scope in MaybeDiscourageAndDisconnect() (master...2020-07-tidyup-maybediscourage) https://github.com/bitcoin/bitcoin/pull/19472
< bitcoin-git> [bitcoin] jnewbery opened pull request #19583: Clean up Misbehaving() (master...2020-07-tidy-misbehavior) https://github.com/bitcoin/bitcoin/pull/19583
< bitcoin-git> [bitcoin] ecurrencyhodler opened pull request #19584: doc: Update obsolete links to online reference #19582 (master...patch-1) https://github.com/bitcoin/bitcoin/pull/19584
< bitcoin-git> [bitcoin] stylesuxx opened pull request #19585: rpc: RPCResult Type of MempoolEntryDescription should be OBJ. (master...19579) https://github.com/bitcoin/bitcoin/pull/19585
< luke-jr> achow101: what else uniquely identifies a wallet? if sqlite doesn't have one, I suggest adding one explicitly from the start..
< cfields_> jnewbery: I just happened to see it here, haven't looked into it fully...
< cfields_> "Change cs_main TRY_LOCK to LOCK" IIRC the TRY_LOCK was _very_ important for making sure that peers are visited fairly.
< cfields_> I believe that change may significantly change how peers are processed.
< sipa> vasild: what do you need varint for? afaik it's not used in any public interface
< cfields_> I'll add it to my review queue.
< luke-jr> achow101: thanks for the explanation of rebase needed; I was quite confused, lol
< jonatack> cfields_: TIL, interesting!
< cfields_> jonatack: I'm struggling trying to remember the details.
< sipa> cfields_: i believe that a long time ago, during ProcessMessages cs_recv was held, and during SendMessages cs_send was held
< sipa> and the network thread would try to grab what wasn't locked, so sends could be done in parallel with ProcessMessages, and receives could be done in parallel with SendMessages, for the same peer
< sipa> i believe that's no longer the case, and SendMessages/ProcessMessages just grab send/recv locks whenever needed
< sipa> in particular, ProcessMessages doesn't lock cs_recv while validating a block...
< cfields_> sipa: maybe, but there's something about taking cs_main and stalling the processing pipeline.
< sipa> nothing in net locks cs_main
< cfields_> sipa: right, but now SendMessages is guaranteed to wait for cs_main for every peer, every run through the loop.
< cfields_> before that would've caused it to hop around peers due to randomly failing to grab the lock.
< cfields_> Now they'll be guaranteed processed in-order.
< sipa> right
< sipa> exactly which PR are you talking about, btw?
< cfields_> Which iirc was explicitly not desired at one point, though I can't remember why :(
< cfields_> #19472
< gribble> https://github.com/bitcoin/bitcoin/issues/19472 | [net processing] Reduce cs_main scope in MaybeDiscourageAndDisconnect() by jnewbery · Pull Request #19472 · bitcoin/bitcoin · GitHub
< sipa> cfields_: ah i see
< sipa> cfields_: i'm not sure this really matters, as nearly every message being processed for incoming messages also grabs cs_main
< cfields_> sipa: sure, for post-handshake peers :)
< cfields_> you're right though. I'm struggling to remember how this could've been significant.
< cfields_> sipa: that's what it is... ProcessMessages and SendMessages are called together, as "we have a message to process" is what we use as a proxy for "maybe we should send some stuff too". Now the SendMessages are all guaranteed to run to the end, so we'll be spending more time in there now than before. I have no idea what the hit/miss ratio looked like before, so I'm not sure if that's significant.
< cfields_> *TRY_LOCK hit/miss
< achow101> luke-jr: the full file path to the wallet should be sufficient
< achow101> As a unique identifier
< sdaftuar> cfields_: does that mean that if a user is doing a bunch of RPC calls that are grabbing cs_main, we might let our receive buffers fill up more?
< sdaftuar> that seems like a plausible reason why the old behavior might have been desirable i guess
< cfields_> sdaftuar: not on the socket side, but on the processing side, yes.
< cfields_> sdaftuar: but like sipa said, something would be bound to take cs_main eventually anyway.
< luke-jr> achow101: not if the user moves it
< sipa> luke-jr: i think it makes sense to be able to give a warning to detect a (fork of) a wallet file being loaded multiple times, but it's not really as necessary as it was with bdb (which would give annoying errors if you had the same db opened twice in the same env)
< cfields_> sdaftuar: the more I think about it, the more I think it was probably just a way of throttling SendMessages to be called less often than every run through the message handler loop.
< sipa> luke-jr: we could do the check at a more semantic level... like do you have multiple wallets watching the same addresses
< cfields_> s/to be called/to fully execute/
< achow101> luke-jr: you're persisting the IDs?
< sipa> cfields_, sdaftuar: in that scenario i think the difference is spinning through SendMessages and blocking on the first ProcessMessages that needs cs_main, vs blocking immediately in SendMessages
< sipa> i don't think that's a big difference
< luke-jr> sipa: this isn't for detecting forks..
< sipa> luke-jr: i mean forks in the history of the wallet file; not blockchain forks
< luke-jr> achow101: yes, that's the whole point
< luke-jr> sipa: right
< luke-jr> sipa: what I'm doing in that PR, is preventing pruning if we know we have a wallet that needs the blocks for syncing
< cfields_> sipa: I agree with that. I drop the cs_main objection. I am still concerned about the time spent in that function, though. Will follow-up with jnewbery for some before/after benchmarks.
< sipa> cfields_: i think post-move-to-more-net_processing-and-it-having-its-own-lock(s), there are more opportunities to do conditional processing anyway
< luke-jr> sipa: once the wallet is synced, we want pruning to continue, so it uses the fileid to track the prune lock
< achow101> luke-jr: I guess you could add an id record..
< sdaftuar> sipa: i think the differene is that in SendMessages, if you were waiting a long time before you get invoked, then you might be sending stale data to your peers
< luke-jr> achow101: bdb already has this, it's just sqlite where we'd need to add it
< luke-jr> which is fine since sqlite has no existing wallets yet
< luke-jr> (and if we migrate bdb to sqlite someday, we can migrate fileid)
< sipa> luke-jr: i'm not really a fan of mixing a dblayer identifier with an application level identifier
< sipa> if you import a descriptors from one wallet file into another, shouldn't that also be treated as a duplicate?
< luke-jr> dunno
< luke-jr> syncing the new one won't sync the old one if you still have it around
< sdaftuar> actually, scratch what i said, i misremembered how those ProcessMessages/SendMessages calls work
< cfields_> sdaftuar: right, the actual sends are happening async :)
< luke-jr> sipa: it's not the keys in the wallet that matter for this per se, but the state of the sync of the wallet
< luke-jr> which isn't really correlated with the descriptor
< sipa> luke-jr: i see
< achow101> luke-jr: I suppose it's fine to use the bdb I'd. But I would prefer if you didn't use WalletDatabaseFileId. A uint160 should work, the id is 20 bytes
< luke-jr> achow101: well, I'm turning it into a std::string in CWallet - maybe I should do that at the WalletDatabase level?
< achow101> That would be preferable
< jnewbery> cfields_: Just catching up on this now. Thanks for looking at the PR.
< jnewbery> My understanding was that the TRY_LOCK was added to fix a potential deadlock between cs_main and cs_vSend, introduced in #1117
< gribble> https://github.com/bitcoin/bitcoin/issues/1117 | Fix potential deadlock by sipa · Pull Request #1117 · bitcoin/bitcoin · GitHub
< jnewbery> I expect that the hit ratio for that TRY_LOCK is almost 1 (ie we get the lock almost always), because other threads don't hold cs_main very much
< jnewbery> unless you're really hammering the RPC with something that takes cs_main, but even then it'd be difficult
< bitcoin-git> [bitcoin] sanjaykdragon opened pull request #19586: REFACTOR: moved from percent format to proper format for consistency (master...master) https://github.com/bitcoin/bitcoin/pull/19586
< yanmaani> What's the purpose of the options objects in RPCs, such as bumpfee?
< yanmaani> couldn't you just leave them as normal root-level named params?
< luke-jr> yanmaani: if you want an ugly interface..
< yanmaani> luke-jr: how do you mean? Aren't named params optional?
< sipa> yanmaani: ootions objects existed before we supported named params
< luke-jr> yanmaani: foo(1, null, null, null, null, null, null, null, null, null, 2) is super ugly
< sipa> and it makes the positional interface pretty annoying
< sipa> as luke-jr points out
< luke-jr> yanmaani: after #17356 I have a commit that allows mixing options in with the usual named params
< gribble> https://github.com/bitcoin/bitcoin/issues/17356 | RPC: Internal named params by luke-jr · Pull Request #17356 · bitcoin/bitcoin · GitHub
< yanmaani> Oh wait, so named args are sort of positional at the same time?
< yanmaani> Couldn't you just have all the named args at the end? Or would you still need nulls
< luke-jr> yanmaani: root-level are positional
< sipa> yanmaani: internally everything is positional
< promag> yanmaani:
< yanmaani> Are named args positional?
< luke-jr> yanmaani: you'd need nulls to get to the option you want
< promag> ops
< sipa> yanmaani: they're mapped to positional arguments internally
< yanmaani> luke-jr: Can't I write option=value?
< luke-jr> positional arguments only make sense for things you would normally specify for every/common use case
< luke-jr> yanmaani: only if you're using named params
< sipa> yanmaani: the external interface supports both named or positional, as json-rpc specifies
< sipa> internally every named argument is just mapped to an internal positional on3
< yanmaani> Ohh. So you don't want them because of that reason?
< sipa> yanmaani: i don't think this matters
< luke-jr> yanmaani: the only reason for args to be root-level, is for them to be used positionally
< yanmaani> If someone is using something where they can only use positional args, then you don't want them to write null, null, null, ...
< sipa> even if things were implemented differently internally, we still want to support a usable.positional interface
< promag> yanmaani: if you read rpc method implementations then you see that params are read with index, like param[2]..
< yanmaani> luke-jr: Well it's a bit cleaner to write rpccall true 1 foo=bar asd=asd than rpccall true 1 '{"foo": "bar", "asd": "asd"}'
< luke-jr> sipa: implementing it differently would enable naming options without an options={…} though
< sipa> luke-jr: right
< luke-jr> yanmaani: JSON-RPC doesn't allow that
< promag> yanmaani: you mean on the console?
< yanmaani> So the names are just cosmetic?
< yanmaani> promag: yeah
< luke-jr> yanmaani: JSON-RPC only allows ALL positional or ALL named
< luke-jr> options is how we can get both
< yanmaani> oh.
< yanmaani> nice
< yanmaani> And if you're using all named, then you still need a fallback all-named API?
< sipa> you mean all-positionalm
< sipa> ?
< promag> rpccall true 1 foo=bar asd=asd <----- you are mixing here
< sipa> tbh, i never use the named interface
< promag> sipa: I though you use brain-ipc-foo
< yanmaani> sipa: yes
< yanmaani> a fallback all-pos
< sipa> right, we need some way of supporting an all-pos interface
< sipa> i wouldn't call it fallback - it's just one of the two supported interfacez
< yanmaani> Oh. Nice.
< sipa> promag: lol
< promag> what was the motivation for named params btw?
< sipa> json-rpc specifies them
< luke-jr> XD
< sipa> and in some contexts, i think they're useful
< promag> it's not mandatory is it?
< sipa> though i hacen't seen much need personally
< promag> for methods with optional params then yeah, named is cool
< sipa> promag: it is not clear from the spec, to me
< sipa> whether thet're required to be supported on the server side or not
< promag> commit -m "drop rpc, add graphql"
< promag> jk, long live json-rpc, its awesome
< sipa> i think we need x86-rpc; the client sends a bit of x86 assembly to the server, which executes it
< sipa> it's extremely flexible!
< promag> ack
< instagibbs> named params are super helpful when you have an rpc call with 9+ args :)