< 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] 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 :(
< 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
< 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