< yanmaani>
Are there any benchmarks on Bitcoin Core's RPC done?
< yanmaani>
If I make a blockchain explorer, will it pose a problem?
< luke-jr>
sipa: this doesn't look exploitable?
< luke-jr>
yanmaani: no, you can't rely on deprecated getrawtransaction behaviour - enable txindex instead
< yanmaani>
luke-jr: Thanks. Can I use that with a pruned blockchain? What RPC does that unlock?
< luke-jr>
no, you can't..
< luke-jr>
I'm not sure the deprecated functionality would work either
< luke-jr>
getrawtransaction uses txindex
< yanmaani>
So there's no alternative to getrawtransaction?
< luke-jr>
to do what?
< yanmaani>
Building a blockchain explorer
< yanmaani>
So you want functionality like: start at txid X, look at addresses A, B, C, look at their txns Y, Z, and so on. So if I want info re: txid X, then I can do getrawtransaction <TXID>
< aj>
a pruned blockchain explorer?
< yanmaani>
aj: Does pruning block me from doing txindex?
< yanmaani>
(no pun intended)
< aj>
pruning removes txs, so you can't explore the whole blockchain? afaik txindex requires a full history but not sure
< yanmaani>
doesn't txindex copy it over to DB?
< luke-jr>
yanmaani: an address should only ever have 1 transaction associated <.<
< yanmaani>
I meant that A had txn Y, and B had txn Z, of course :)
< aj>
yanmaani: txindex is an index -- it helps you find stuff in the database. if you remove stuff from the database it can't be found, no matter how good your index is
< yanmaani>
Is there anything like txindex for addresses?
< aj>
some PRs have been proposed for an address index
< aj>
hmm, has anyone gotten llvm memory sanitizer to work? seems to give a pretty quick failure in boost test setup for me
< luke-jr>
aj: Isn't that the one that requires you to rebuild all your libraries too?
< sipa>
yes it is
< aj>
ah, that would explain it then
< sipa>
generally you do (ubsan and asan) or (tsan) or (msan, including all libs)
< yanmaani>
How come gettxout works with all txids, but gettransaction only does wallet txns?
< yanmaani>
Is there any PR/patch to add a TXO index?
< achow101>
yanmaani: gettxout is a node rpc and accesses the txout set. it is limited to utxos. gettransaction is a wallet rpc, it accesses the wallet. it is limited to things in the wallet. you can use getrawtransaction to get arbitrary transactions, and enable txindex to get any arbitrary transaction
< achow101>
note that getrawtransaction is also a node rpc and will not retrieve wallet transactions if they are not unspent and txindex isn't enabled
< yanmaani>
achow101: getrawtransaction, that's the one thank you so much!
< bitcoin-git>
[bitcoin] S3RK opened pull request #20191: wallet, refactor: make DescriptorScriptPubKeyMan agnostic of internal flag (master...remove_m_internal) https://github.com/bitcoin/bitcoin/pull/20191
< vasild>
extended tests may still be run by this CI?
< jnewbery>
aj: so I guess you're happy to keep it at the same time. It'll be an hour earlier for northern hemisphere folks, which might be upsetting for those on the US west coast
< jnewbery>
vasild: ah, you'd also need to add the name of your test to the end of that config line
< vasild>
What I tried is: I added a custom option to the script "--forcerun" and if it is not set then I raise SkipTest()
< vasild>
but the test is still run by travis, and I have no clue why. It must have detected the option and run it with that option set...
< vasild>
I tried running "test_runner.py --ci" locally, and the test is skipped as expected
< vasild>
jnewbery: that would be quite ugly, no?
< bitcoin-git>
[bitcoin] fanquake opened pull request #20195: build: fix mutex detection when building bdb on macOS (master...bdb_xcode12_implicit_function_decleration) https://github.com/bitcoin/bitcoin/pull/20195
< jnewbery>
vasild: People can live with ugly travis config I think :)
< vasild>
jnewbery: ok, maybe ugly was not the right word, but it is also fragile because it means that the test will be run whenever "test_runner.py --extended" is executed
< vasild>
and it will fail
< vasild>
sometimes I run "test_runner.py --extended" locally
< vasild>
I guess other people also do and I do not want to impose "always run with --extended --exclude foo" otherwise you will get failure
< bitcoin-git>
[bitcoin] vasild opened pull request #20196: net: fix GetListenPort() to derive the proper port (master...fix_GetListenPort) https://github.com/bitcoin/bitcoin/pull/20196
< vasild>
jnewbery: I figured it out - had to move the check earlier - from run_test() to setup_nodes()
< bitcoin-git>
[bitcoin] jonatack closed pull request #20193: p2p: practicalswift would like review of 19972 (master...evict-inbound-onions) https://github.com/bitcoin/bitcoin/pull/20193
< jnewbery>
vasild: good!
< bitcoin-git>
[bitcoin] jonatack opened pull request #20197: p2p: improve onion detection in AttemptToEvictConnection() (master...AttemptToEvictConnection-identify-onions-with-m_inbound_onion) https://github.com/bitcoin/bitcoin/pull/20197
< jonasschnelli>
MarcoFalke: I saw that you have merged some PRs in the GUI repository. Are you going to open a PR on the main repository?
< jonasschnelli>
with the settings.json file: is there a way to disable loading a specific wallet that has been stored in settings.json?
< ryanofsky>
jonasschnelli, if you run with -nowallet on command line it resets the list and takes precedence, and you can add other wallets after like -nowallet -wallet=mywallet
< ryanofsky>
also wallet is removed from settings if you unload it in the gui or call an rpc with load_on_startup=False
< ryanofsky>
and #20186 will avoid creating the wallet, if you are trying to avoid creating, not loading
< jonasschnelli>
ryanofsky: settings.json is only in conjunction with the GUI, right? RPC loadwallet will not make create that file?
< ryanofsky>
no problem. also you can run with -nosettings to ignore the whole file
< ryanofsky>
yes by default loadwallet doesn't modify settings unless you pass load_on_startup=true or load_on_startup=false
< jonasschnelli>
I guess the issue if someone creates a wallet with --wallet=mywallet in bitcoin.conf or via cli parameter, unloaded it and loads it again in the GUI (or with load_on_startup), restarts bitcoin with the same parameter will lead to a halt due to a duplicate -wallet parameter?
< jonasschnelli>
(the issue can be overlooked / edge-case)
< ryanofsky>
i think ideally duplicate wallet error would not be triggered by that, but it is an edge case
< ryanofsky>
there maybe be similar cases worth fixing. like if a bitcoin.conf specifies a wallet to load, and you unload it and reload it in the gui, that should not trigger any duplicate wallet error
< ryanofsky>
but in general the idea is for the gui to simply reload the same wallets you loaded last time, so you can stop using bitcoin.conf
< jonasschnelli>
ryanofsky: the reason why I look at that issue is that probably some users have specified --wallet(s) in bitcoin.conf (or CLI parameter) and will eventually run into the load/unload issue
< jonasschnelli>
They are potentially puzzled why the "duplicate error" pops up and core refuses to start
< ryanofsky>
right, i'm agreeing that should be fixed
< jonatack>
ryanofsky: i wonder if adding mention of -nowallet to -wallet would be helpful. like how -nosettings is documented in -settings.
< jonasschnelli>
it's more an upgrade issue than a "I'm new to 0.21" thing
< jonasschnelli>
ryanofsky: would it hurt to just de-duplicate those entries (ignore duplicates)?
< ryanofsky>
jonatack, could be depending on the use case. I thought -nosettings was useful to document for a sysadmin who doesn't wants a static configuration and doesn't want dynamic settings being used
< ryanofsky>
Use cases for -nowallet are more obscure as far as I know. We use them in the testing framework, and here we are talking about using it to work around a bug that should just be fixed
< jonatack>
ryanofsky: oh ok. the "no" idiom is known, but TIL you can reset the wallet list with -nowallet
< ryanofsky>
s/doesn't wants/just wants/ above
< jonasschnelli>
I also wasn't aware of the -nowallet reset in the parameter order jonatack
< jnewbery>
#startmeeting
< lightningbot>
Meeting started Tue Oct 20 15:00:45 2020 UTC. The chair is jnewbery. Information about MeetBot at http://wiki.debian.org/MeetBot.
< jnewbery>
Before we do that, does anyone have any updates or want to share what they're working on/prioritising?
< jnewbery>
ok, our single topic is:
< jnewbery>
Remove timestamps from addr messages? It seems like the timestamp is only used to leak information about our recent connectivity. It doesn't look like we use it to make decisions about who to connect to. (sdaftuar/jnewbery)
< jnewbery>
sdaftuar: do you want to explain?
< sdaftuar>
oy, i can try
< sdaftuar>
i guess the background here is around looking at how addrman works, and what information it might leak about our peers (and whether or not that is ok, i guess)
< emzy>
hi
< sdaftuar>
i was chatting with jnewbery about the interaction specifically with block-relay-only peers, where we really don't want to leak anything
< sdaftuar>
and one observation was that right now in master, we basically directly leak the time at which we were connected to a block-relay-only peer after we disconnect from that peer and then include the address in a getaddr response
< gleb>
It does indeed leak info, but I never thought about solving the issue in this fashion...
< sdaftuar>
we can fix that, but it led to wondering: what good does this time do anyone, anyway?
< sipa>
is it actually not used for anything?
< sdaftuar>
our own software seems to barely use those times: we use it to sometimes filter out responses to getaddr requests, and we use to sometimes to evict things from the new table
< gleb>
There is a check in IsTerrible
< gleb>
Seeing if it’s older than a month
< ariard>
the fact it's not used by core addrman doesn't mean it's not used by some other bitcoin clients to decide its peering
< sdaftuar>
but we do not use it for determining who to connect to, as far as i can tell.
< gleb>
From what I remember I think you are right, but those existing features are not nothing
< sdaftuar>
ariard: agreed. but i thought it might be worth polling people about how much good these timestamps can do, as they are necessarily unverifiable data?
< jonatack>
CAddrMan::Good: "nTime is not updated here, to avoid leaking information about currently-connected peers."
< sipa>
sdaftuar: good question
< sdaftuar>
gleb: i imagine we could replace their use in those two places without that much trouble
< gleb>
I think the way we use them, an adversary can’t really manipulate, because the checks are very moderate
< sdaftuar>
eg by using nlasttry/nlastsuccess
< gleb>
even if someone bumps their own addrs too much, they don’t really become “better”
< gleb>
sdaftuar: and that won’t propagate through the network?
< gleb>
it will have effect only locally at every node
< sdaftuar>
gleb: given that we don't use them for much, it seems there is only downside to us by potentially telling our peers who we were connected to and at what time?
< gleb>
Telling that a given address we’re relaying is not one year old...
< sdaftuar>
we could still use nlastsuccess to filter out old addresses from our getaddr responses, i think?
< ariard>
sdaftuar: right, unverifiable data doesn't mean it can be useful even if it's gentleman-style of enforcment
< ariard>
a lot of alternatives p2p stack doesn't sanitize their addrs with a feeler connection
< gleb>
and they can retell this fact to other peers without connecting by themselves. Just tell what we told
< sdaftuar>
interestingly, we don't update that time field when we successfully connect to a peer via a feeler connection, i believe.
< gleb>
I need to look better at the code. I’d be very happy to get rid of this stuff
< sipa>
i'll also have a look in more detail
< jnewbery>
ariard: tradeoff there seems to be between helping a [theoretical] alternative implementation make better decisions about who to connect to -vs- protecting our own privacy
< sipa>
it's very appealing
< jnewbery>
by default we should always lean towards protecting our own privacy, unless doing so would be detrimental to the network as a whole
< ariard>
jnewbery: I would favor protecting our own privacy, but as a good practice asking on the ml would be great, at least to warrant
< gleb>
jnewbery: I suggest ariard thinks more whether time stamps can help light clients a lot comparing to feeler strategy
< amiti>
+1 I don't have a complete understanding of these timestamps, but when I've looked at them in the past I've come to similar conclusions where they aren't used for much since they are unreliable and are easy to accidentally leak information
< jnewbery>
ariard: +1. This would be a de facto change to the p2p protocol. Circulating it on the mailing list would be good manners, at least
< gleb>
They are unreliable, but they also can’t be exploited actively I think (only leak info, passive exploit)
< sdaftuar>
gleb: whether they can be exploited depends on how people are using them. i agree our software seems to be designed so that this is just an information leak
< jnewbery>
I think another piece of (almost) useless data that we could stop sharing is the start_height in the version message, but that's maybe a different discussion
< amiti>
yeah, exactly, they can't be exploited because we write logic to not rely on it
< ariard>
Even assuming they're used by some lightclient p2p stack, inviting ecosystem-wise not relying on them due to their distrusted nature would be better
< sipa>
we can stop using the nTime field without caring about what others do... if we'd start setting them differently (all zero? just the current time? random within some window?) we may want to seek opinions on the ml
< sdaftuar>
agreed
< sipa>
i imagine we'd do those at different points in time anyway
< gleb>
Sipa: but Most of their use is already about setting them differently, it seems we mainly discussing dropping that :)
< gleb>
or, well, using them to filter out responses I guess. We sort of “promised” to use them?
< gleb>
whatever, i think we’re on the same page
< ariard>
just set them to zero, in case of randomness source breakup that's not a fingerprint for your node
< gleb>
Setting them to 0 would break compatibility
< sipa>
ariard: if our RNG has issues, we have bigger problems
< gleb>
old nodes think that ntime < 10000000 is trash iirc
< sipa>
ariard: and setting them to 0 would actively hurt relay chances on current code
< gleb>
we probably should randomize them within a week window from now or so
< ariard>
good to know, do we have other compatibility bounds to care about beyond ntime < 10000000 ?
< sdaftuar>
i have another related topic to mention while we're discussing addrman-- i opened a PR to fix some interactions between addrman and block-relay-only peers. it's a reversal from the direction i was leaning before about how this should work, so wanted to mention it in case anyone wanted to discuss
< sdaftuar>
cc amiti and jnewbery as this came up during a recent PR under review
< jnewbery>
#20187
< gribble>
https://github.com/bitcoin/bitcoin/issues/20187 | Addrman: test-before-evict bugfix and improvements for block-relay-only peers by sdaftuar · Pull Request #20187 · bitcoin/bitcoin · GitHub
< sdaftuar>
the tl;dr is that after looking into how eviction works from the new and tried tables, i decided it all works better to make sure that our block-relay-only peers in fact get moved to the tried table
< sdaftuar>
which necessitates invoking addrman functions on those addresses and changing addrman state of course
< jnewbery>
the change in net_processing seems reasonable to me. I haven't looked at the changes in net.
< sipa>
right
< sdaftuar>
but lots of things to consider (particularly privacy issues that are hard to reason about) so if someone spots a problem i'd love to discuss
< sdaftuar>
one particular problem is if the timestamps we return in getaddr messages for those peers will stick out somehow!
< sipa>
it's a balance beteeen not updating addrman to minimize detectability of block-only connections, and updating it to make sure we keep good ones
< bitcoin-git>
[bitcoin] jonasschnelli opened pull request #20198: Show name, format and if uses descriptors in bitcoin-wallet tool (master...2020/10/wallet_tool_sqlite) https://github.com/bitcoin/bitcoin/pull/20198
< sdaftuar>
yep
< amiti>
the idea makes sense, I'll take a closer look at the code
< sdaftuar>
(that's all i've got)
< jnewbery>
While we're on the subject of addrman, it seems strange to me that it's owned by CConnMan. I think it makes sense to pull it out into a separate component that's owned by the node context object, so other components can access it directly.
< sipa>
sdaftuar: do you believe there are more issues than fixed by your PR?
< jnewbery>
Is there areason not to do that?
< ariard>
what other components need access to addrman ? or might need in the future?
< sipa>
jnewbery: whatever works
< jnewbery>
ariard: net_processing and rpc
< gleb>
No opinion on moving components around
< sdaftuar>
sipa: not at the moment, i dont' think. the only other addrman-related thing i'm worrying about is addr relay i think
< sipa>
and net
< sdaftuar>
but that's a different type of issue
< jnewbery>
currently net_processing access addrman through some forwarding functions in cconnman
< ariard>
sounds good to move so
< jnewbery>
any other topics before we wrap up? Anyone have any review begs?
< ariard>
what outstanding p2p bugfixs/followups are required for current release ?
< jonatack>
I plan to circle back soon to finish reviewing #19858 which looks pretty close
< luke-jr>
another example would be backup reminders
< achow101>
i still don't see how not having a preexisting id is a problem. If you add the id to a wallet, and add the prune lock for that id, then we won't prune beyond the requirement for that wallet. when the backup is restored, sure a new id is generated, but we still haven't pruned too far
< achow101>
it's just extra stuff in the db
< luke-jr>
and have a poor UX because we have the wallet twice in prune locks confusing the user
< luke-jr>
nevermind unknown future use cases
< achow101>
sure, but a unique wallet id should not be at the db level
< sipa>
having a warning for the user that two copies of the same wallets have been loaded seems moderately useful in any case
< achow101>
we could make the id deterministic based on active spkman
< luke-jr>
actually, right now users expect an error if they try to load two copies..
< achow101>
for legacy, use the current seed, or default key for the non-hd wallets as they still have default key. for descriptor, hash the active descriptors
< sipa>
luke-jr: i usually just find it annoying that i can't load them at the same time :)
< sipa>
but a warning seems useful
< achow101>
the error for loading duplicates is because bdb has issues when duplicates are loaded, not for any functionality reason
< sipa>
i know
< luke-jr>
achow101: "current" anything would be wrong since it can change
< sipa>
luke-jr's argument is that users have come to rely on that behavior... i'm not convinced it is, but if it is, i think just documenting it in release notes that the same doesn't hold for sqlite/descriptor wallets is fine
< achow101>
luke-jr: if it changes, then you have a new backup..
< achow101>
it's arguably not the same wallet
< achow101>
luke-jr: you could also just prompt people to make a backup of their wallet(s) when they first use prune locks
< luke-jr>
more bad UX for no reason
< achow101>
let me rephrase my earlier suggestion: for wallets missing a unique id, compute one based on the contents of that wallet (e.g. seed, default key, descriptors) at that time.
< achow101>
for valid backups of that wallet, the id will be the same. otherwise that backup is useless
< achow101>
this id persists, so later backups will have the same id
< jonasschnelli>
hebasto: yes. now. But not for long.
< luke-jr>
achow101: but older backups might not
< luke-jr>
might as well just fix it now and avoid the mess later entirely
< achow101>
luke-jr: why would they not?
< luke-jr>
achow101: perhaps the user has rotated their seed
< achow101>
then they made a new backup with the new seed
< achow101>
and should be restoring that one
< luke-jr>
oh, and since Core refuses to consider compatibility with non-Core wallets, this could mean it's not safe to use it until Core releases prune locks itself
< achow101>
huh?
< luke-jr>
achow101: for example, I couldn't put sqlite wallet + prune locks in Knots without a risk Core breaks it in the future
< luke-jr>
Knots already has BDB + prune locks, which is safe because it doesn't require any wallet format changes
< bitcoin-git>
[bitcoin] hebasto opened pull request #20206: wallet, refactor: Include headers instead of function declarations (master...201020-headers) https://github.com/bitcoin/bitcoin/pull/20206
< achow101>
jonasschnelli: I don't know what's wrong with that failure
< bitcoin-git>
[bitcoin] sipa opened pull request #20207: Follow-up extra comments on taproot code and tests (master...202010_taproot-comments) https://github.com/bitcoin/bitcoin/pull/20207