< gribble>
https://github.com/bitcoin/bitcoin/issues/18550 | Store destdata for change in separate key for backward compatibility by luke-jr · Pull Request #18550 · bitcoin/bitcoin · GitHub
< ryanofsky>
meshcollider: i think both prs should be dropped, but first pr is basically harmless, only second pr has bad side effects
< wumpus>
meshcollider: the logic is like: *IF* we need a rc2, might as well include them, I think
< wumpus>
I don't think they necessiate a rc2 by themselves
< wumpus>
but generally for major releases we have around 4-5 rcs so ok
< wumpus>
but we could remove the milestone from it if people prefer that (especially if they have bad side effects), or generally if they don't get enough ACKs
< meshcollider>
I don't think they've even got the milestone, it was just mentioned in the OP
< meshcollider>
First PR is harmless but also pointless
< jonatack>
meshcollider: fwiw #17824 has acks by kallewoof and myself and review by instagibbs and achow101
< meshcollider>
jonatack: cool thanks, I'll look at it first thing tomorrow morning, it's getting late here :)
< jonatack>
meshcollider: 👍
< meshcollider>
ryanofsky: After reading through your comments, I agree with you, I'll close both the PRs
< meshcollider>
If Luke has a strong argument that we are missing, we can discuss in the meeting tomorrow
< bitcoin-git>
[bitcoin] meshcollider closed pull request #18550: Store destdata for change in separate key for backward compatibility (master...changedata) https://github.com/bitcoin/bitcoin/pull/18550
< bitcoin-git>
[bitcoin] meshcollider closed pull request #18572: Wallet: Accept "changedata" db key as an alias to "destdata" (master...changedata_forwardcompat) https://github.com/bitcoin/bitcoin/pull/18572
< jonatack>
Next wallet meeting is April 24 (in 8 days) if not mistaken
< jonatack>
(we did one last week)
< aj>
on this side of the world, regular meeting is tomorrow
< fanquake>
aj: tossing a coin on attendance
< meshcollider>
Yeah sorry I mean the regular meeting which is "today"
< bitcoin-git>
[bitcoin] hebasto opened pull request #18665: Do not expose -logthreadnames when it does not work (master...200416-logthreads) https://github.com/bitcoin/bitcoin/pull/18665
< bitcoin-git>
bitcoin/master bee88b8 James O'Beirne: tests: have coins simulation test also use CCoinsViewDB
< bitcoin-git>
bitcoin/master d8dfcea MarcoFalke: Merge #17669: tests: have coins simulation test also use CCoinsViewDB
< bitcoin-git>
[bitcoin] MarcoFalke merged pull request #17669: tests: have coins simulation test also use CCoinsViewDB (master...2019-12-coins-tests) https://github.com/bitcoin/bitcoin/pull/17669
< bitcoin-git>
[bitcoin] theStack opened pull request #18672: test: add further BIP37 size limit checks to p2p_filter.py (master...20200416-test-add-further-bloom-filter-size-limit-checks) https://github.com/bitcoin/bitcoin/pull/18672
< wumpus>
checking the minimum libevent version in configure would make sense
< wumpus>
the build system has historically not checked minimum versions at all as far as I know, but it'd be a good thing to do, bigger chance that someone will figure out what is the problem in that way than having to check a separate documentation file
< wumpus>
libevent 2.0.21 is *ancient* though
< wumpus>
8 years old now
< wumpus>
things move on and we can't support old software forever, that's just not realistic
< luke-jr>
the old version isn't what matters for such questions IMO
< luke-jr>
but rather, how old/widespread is the new minimum?
< wumpus>
there is no new minimum
< wumpus>
this is about enforcing the current minimum in configure.ac
< wumpus>
which has been 2.0.22 pretty much forever
< luke-jr>
afaik that's pretty easy
< MarcoFalke>
I don't understand how Bitcoin Core compiles fine, but the fuzz tests don't
< wumpus>
I don't, either, but please just use a newer libevent version, many bugs have been fixed since
< MarcoFalke>
Yeah, fuzzing old libevent isn't that helpful
< meshcollider>
Meeting?
< luke-jr>
(let's get to the wallet issue this time)
< achow101>
meeting?
< wumpus>
#startmeeting
< lightningbot>
Meeting started Thu Apr 16 19:03:58 2020 UTC. The chair is wumpus. Information about MeetBot at http://wiki.debian.org/MeetBot.
< gribble>
https://github.com/bitcoin/bitcoin/issues/18550 | Store destdata for change in separate key for backward compatibility by luke-jr · Pull Request #18550 · bitcoin/bitcoin · GitHub
< MarcoFalke>
achow101: Assigned milestone. I think this will be dealt with in rc2
< wumpus>
yes, if it's required for windows signing on some environments it definitely needs to go into the next rc
< wumpus>
I didn't need it for LXC fwiw
< jonasschnelli>
me2
< MarcoFalke>
happens only on docker/podman
< hebasto>
yes, lxc does need it
< achow101>
yeah, seems to be a virutaliation specific issue
< achow101>
just wanted to make sure no one forgot about it
< hebasto>
achow101: mind specify it in pr description?
< wumpus>
yes, good point
< jonatack>
i think i ran into it when win gitian signing with docker
< achow101>
hebasto: done
< wumpus>
makes sense
< wumpus>
luke-jr: I think there have been some disagreements about your PRs, meshcollider was concerned about #18550 for example
< gribble>
https://github.com/bitcoin/bitcoin/issues/18550 | Store destdata for change in separate key for backward compatibility by luke-jr · Pull Request #18550 · bitcoin/bitcoin · GitHub
< wumpus>
it's probably too late in the release cycle to do changes like that
< luke-jr>
wumpus: are we changing to that topic now? I can wait a bit
< luke-jr>
it's a bugfix
< MarcoFalke>
What bug is it fixing? You didn't include any regression tests
< meshcollider>
ryanofsky should really be here for this discussion too
< ryanofsky>
here
< luke-jr>
MarcoFalke: I don't see how to make tests for this kind of thing
< sipa>
luke-jr: can you give an exact scenario for reproducing the issue?
< luke-jr>
wumpus: yes, 0.9
< luke-jr>
sipa: set a destdata on change, then use the wallet in an older version
< sipa>
assuming no further changes related to the topic go into 0.20
< wumpus>
why does it now suddenly need to be changed before 0.20
< sipa>
what does a user do to cause "set a destdata on change" ?
< ryanofsky>
luke-jr, that is a pre-existing bug in 0.19
< luke-jr>
sipa: currently, it is possible only be using an avoid-reuse wallet
< ryanofsky>
it already sets destdata on change and then starts treating it as nonchange
< luke-jr>
only by*
< achow101>
luke-jr: IMO it isn't worth trying to fix this display bug with users who upgrade then downgrade
< MarcoFalke>
I'd say that anything that is not a regression can wait for 0.20.1 or 0.21.0
< luke-jr>
achow101: it will come back when downgrading 0.21 to 0.20
< ryanofsky>
it'd be easy to backport a trivial fix for that issue in the 0.19 branch, but this PR only makes the bug and introduces other bugs
< achow101>
because it's already an issue for those users using the old software, this isn't a regression
< luke-jr>
unless we specifically exclude "used" from the fix in 0.21
< ryanofsky>
*masks the bug
< sipa>
what is the impact? is it just the change address showing up in the address book, or more?
< luke-jr>
sipa: just that, I think
< luke-jr>
(which can be serious IMO)\
< achow101>
sipa: it may have an effect on coin selection
< achow101>
in particular trusted change vs untrusted non-change
< luke-jr>
achow101: well, if it's spent…
< sipa>
my understanding is that it does not affect IsChange, unless a user also goes to set an explicit label on one of those (now shown) change addresses?
< wumpus>
who uses the address book for receiving addresses anyhow
< luke-jr>
sipa: it does
< luke-jr>
sipa: when the change is used, it gets a "" label
< sipa>
i see
< luke-jr>
wumpus: uh, everyone?
< sipa>
i certainly don't, but i don't think that's an important question; if we have an address book, it should work
< wumpus>
luke-jr: I must admit I haven't used the address book *at all* for years, but last time I did it was to check an address I sent to
< luke-jr>
sipa: this gets worse if we add any new features using destdata - for example, marking which destination are actually used to provide warnings on reuse
< ryanofsky>
this is right but it's a preexisting bug, 0.19 already marks addresses used and has the same bug
< achow101>
CAddressBook effects more than just the address book IIRC
< luke-jr>
wumpus: how do you even receive without using it now?
< sipa>
it affecting IsChange is more concerning to me
< wumpus>
luke-jr: create new receiving address in the receive tab, give it out
< luke-jr>
wumpus: that uses the address book..
< wumpus>
no, it doesn't, the 'payment request' table is separate
< sipa>
ok, this is a semantics discussion; this is not productive
< MarcoFalke>
we should just remove the wallet and gui. Doesn't that solve the problem?
< luke-jr>
-.-
< luke-jr>
ryanofsky was proposing removing destdata entirely, but that breaks compatibility in new ways, and loses a big feature IMO
< ryanofsky>
no, that is not my proposal
< ryanofsky>
my proposal is a refactoring that does not change behavior in any way
< ryanofsky>
and is meant to prevent bugs like this in the future, but it's basically orthogonal to what we are talking about
< achow101>
I think the question is whether we will try to fix this in the future in a way that allows downgrading to 0.19
< sipa>
so am i correct that this triggers only if a user on 0.19 has avoid-reuse on, and then uses 0.19 or 0.20?
< ryanofsky>
and have 3 solutions for dealing with it
< wumpus>
destdata was mainly introduced to store payment request data and such, if it's no longer needed it should be removed
< wumpus>
but not for 0.20
< ryanofsky>
my PR is separate and compatible with all 3 approaches
< sipa>
or also when they use 0.20 and then downgrades in certain cases?
< luke-jr>
sipa: it's unfixable for already-released 0.19; my hope is to fix it so downgrading to 0.20 works
< luke-jr>
wumpus: it's still needed
< luke-jr>
destdata is nice because you don't need to upgrade wallets to get features using new metadata
< achow101>
to be clear, is the issue *only* with downgrading?
< luke-jr>
achow101: yes, which is something we've always tried to support for wallets
< tryphe>
address book should be removed outright imo, metadeta that can be modified in a locked wallet is kind of silly
< achow101>
If we didn't care about dowgrading at all, this would not be an issue?
< ryanofsky>
the issue is not only with downgrading, but the fix only deals with downgrading, and is only partial
< ryanofsky>
and it introduces other bug of avoding-reuse feature in 0.19 being broken
< bitcoin-git>
[bitcoin] jnewbery opened pull request #18675: tests: Don't initialize PrecomputedTransactionData in txvalidationcache tests (master...2020-04-precomputedtransactiondata-txvalidationcache) https://github.com/bitcoin/bitcoin/pull/18675
< achow101>
ryanofsky: what's your pr?
< sipa>
can someone give me an exact user scenario that results in a user observing something incorrect, where they start with 0.20.0rc1 as we have now (and then do thing, or downgrade, or ...)
< ryanofsky>
again my pr is orthogonal, refactoring only #18608, comptaible with any dataformat we chose now or in the future
< wumpus>
tryphe: I think that's unrelated; wallet encryption in bitcoin core only protects private keys, nothing more. Labels and such are not part of that either and should definitely be kept.
< luke-jr>
sipa: create avoid-reuse wallet; send a transaction that has change; spend that change; suddenly that change is no longer change
< ryanofsky>
the scenario is just a change address gets used, and 0.19 software treats all addresses marked as used as nonchange
< ryanofsky>
this is a pre-existing scenario that has been around since 0.19
< ryanofsky>
and the only way to fix it reliably is with backports
< luke-jr>
sipa: the last step is only in 0.19
< luke-jr>
(the result)
< luke-jr>
sipa: but if we fix this in 0.21, 0.20 would no longer see the "used" flag unless we special-case it
< sipa>
luke-jr: that very much depends on how it is fixed
< ryanofsky>
luke's pr is a partial fix for the the issue because it stops marking addresses as used in a way 0.19 understands, and marks them used in a different way it is blind to
< sipa>
my concern right now is behavior that is observable with 0.20
< ryanofsky>
there are 0 bugs observable with 0.20
< luke-jr>
I don't see a way to fix it later, without special-casing 0.20 support
< wumpus>
tbh if it's only a minor backwards compatibility issue that is only apparent in downgrading, I wouldn't want to do things substantially different because of it
< tryphe>
wumpus, mostly agree, although allowing modification of metadata of someone's wallet while in cold storage that they might assume stays constant opens up a lot of doors for bad actors
< achow101>
How about we revert the "fix" in 0.20 and try again for 0.21? It seems like ~no one has complained about this bug in 0.19
< ryanofsky>
the fix in 0.20 is correct and causes no backwards compatiability issues
< sipa>
luke-jr: if after that scenario (create avoid-reuse in 0.20; create change in 0.20; spend change in 0.20; downgrade to 0.19; do things), the user upgrades again to 0.20, is the issue resolved, or might it persist?
< meshcollider>
I don't think that's a good idea, the fix is still an improvement
< luke-jr>
achow101: leaving it as-is now is strictly less bad than reverting I think
< sipa>
tryphe: please stay on topic
< wumpus>
tryphe: if that's your concern, encrypt the entire wallet file or store it on an encrypted file system, it's not something bitcoin core's wallet encyrption solves
< ryanofsky>
the fix causes no issues whatsoever
< ryanofsky>
i mean the fix that's already been merged causes no issues, the new fix proposed is a different story
< luke-jr>
sipa: the issue remains resolved in 0.20, provided the user doesn't see it in 0.19 and set a label or something
< sipa>
luke-jr: thank you
< luke-jr>
(in fact, I think 0.19 making the broken state itself, would also appear correct in 0.20)
< wumpus>
okay
< sipa>
that's good to hear as well
< wumpus>
yes, that's good to hear
< sipa>
i think we should document the issue in the release notes, with the point that if the issue appears, upgrading (again) to 0.20 will fix things unless a user manually set a label on an errorneously-shown address in 0.19
< luke-jr>
my biggest "end result" concern, is that I don't think users should need to -upgradewallet to get address reuse warnings when we finally merge that ; but that's a few steps into the future
< sipa>
for 0.21, we can discuss solutions - whether those involved -upgradewallet or special-casing the 0.20 behavior
< achow101>
luke-jr: with what is currently merged, why is changing the fix necessary?
< achow101>
as in why is your proposed 0.21 fix necessary
< luke-jr>
achow101: it's one thing to break compatibility with <0.19 only for avoid-reuse wallets>, another entirely to break compatibiltiy with <normal wallets going back forever>
< luke-jr>
achow101: since "used" destdata is only in avoid-reuse, we have the former situation right now
< luke-jr>
but as soon as we add destdata for change on normal wallets, we get the latter
< achow101>
are we adding destdata for change on normal wallets?
< luke-jr>
achow101: that's my plan for address reuse warnings
< ryanofsky>
achow101, yes, we've been doing that since kallewoof implemented avoidreuse
< wumpus>
just a reminder that we have to reserve some time for MarcoFalke's topic as well
< jonatack>
i agree with sipa's proposals (and with meshcollider and ryanofsky that it's better to keep fix in luke-jr's merged pr)
< luke-jr>
that's how it can avoid needing a -walletupgrade
< achow101>
ryanofsky: that's only for avoid reuse
< achow101>
luke-jr: i see, so what if we don't do that?
< achow101>
and just don't change it
< luke-jr>
achow101: I don't understand what you mean
< luke-jr>
if we don't add address reuse warnigns, we don't have address reuse warnings..
< achow101>
why do we have to add destdata on change for normal wallets
< luke-jr>
to mark the change address as used
< achow101>
if the wallet doesn't signal avoidreuse, then there's no need?
< luke-jr>
address reuse warnings are for all wallets
< ryanofsky>
option 1 is keep using "destdata" record
< ryanofsky>
option 2 is switch to "changedata" record
< sipa>
also, can't the usage data be computed at runtime from other wallet data?
< ryanofsky>
option 3 is "useddata" record
< luke-jr>
sipa: historical best practice?
< sipa>
?
< luke-jr>
sipa: maybe in this case (I haven't looked into that option yet, but I plan to), but this will probably come up again either way
< meshcollider>
Using a different record would be fine, only a minor code complication
< luke-jr>
sipa: we've always tried to not change wallet format outside of an explicit -upgradewallet
< meshcollider>
I dont think it would come up again luke-jr
< luke-jr>
meshcollider: tax metadata for example
< sipa>
luke-jr: it sounds to me like no wallet format change is needed *at all* for this functionality
< meshcollider>
Again use a different record, not destdata
< luke-jr>
meshcollider: what different record?
< sipa>
i think this is taking us too far
< luke-jr>
meshcollider: adding a new one is a wallet format change..
< sipa>
so be it?
< achow101>
luke-jr: we've added/changed records before without needing upgradewallet
< wumpus>
please wrap up this topic
< meshcollider>
In a backwards compatible way if it's new
< sipa>
if it's a record old wallets can just ignore, no need for upgradewallet
< luke-jr>
achow101: we have?
< sipa>
otherwise, use it
< luke-jr>
ok then
< meshcollider>
Yes the topic is done, the PRs are closed as noone else concept ACKs the change
< achow101>
luke-jr: see UpgradeKeyMetadata
< wumpus>
#topic experimental libmultiprocess, next steps for multiprocess in general (MarcoFalke)
< MarcoFalke>
Background is that libmultiprocess has been added to ./depends , and libmultiprocess compile and link flags have been added to our makefile. Everything was opt-in but after merge discussion concluded that it was too early to do this step, so the change has been reverted. I (and ryanofsky) would like to hear and discuss everyone's concerns about libmultiprocess as part of the meeting for brainstorming.
< MarcoFalke>
cc cfields
< cfields>
^^
< MarcoFalke>
cc fanquake (probably sleeping)
< sipa>
my impression is that we should only merge changes for this if the plan is that eventually this will end up in release binaries
< sipa>
it's not clear to me if that's the case
< cfields>
fanquake listed a bunch of really good questions, and ryanofsky has given good answers to on #18588.
< MarcoFalke>
sipa: Eventually that was the goal (at least as I understood it)
< ryanofsky>
that is the goal as far as i know
< MarcoFalke>
sipa: Though, there was no timeline for it
< cfields>
sipa: yes, that was exactly my concern as well. The path forward isn't clear enough to be merging it in in-parts, imo.
< wumpus>
sipa: yes, I think that's obvious
< sipa>
i haven't paid that much attention to the multiprocess project as i don't really care about it (and kind of dread pulling in extra dependencies like capnproto), but i assumed that it was something lots of people wanted - which is fine
< wumpus>
though it's probably ok to have it experimental for a while before it ends up in release binaries
< ryanofsky>
maybe relevant: it builds new binaries
< sipa>
wumpus: sure
< wumpus>
I'm not really sure I like importing capnproto either, just now we got rid of google serialization thing
< ryanofsky>
so a theoretical release could include existing binaries, alongside new multiprocess binaries that let you start and stop node/gui/wallet separately and connect each other
< wumpus>
then again it's probably better than hand-rolling everything
< MarcoFalke>
The multiprocess and capnproto will stay opt-in unless it is decided to change it that
< luke-jr>
ryanofsky: can the same binary do single-process and multiprocess?
< MarcoFalke>
luke-jr: No, they are two binaries
< luke-jr>
I mean hypothetically
< wumpus>
ryanofsky: I'm not sure I like that solution, couldn't we emulate the old way using the new binaries without shipping everything twice?
< ryanofsky>
yes, I just didn't add the option
< ryanofsky>
there is lots of flexibility here
< wumpus>
with the static builds we do that's expensive
< ryanofsky>
I just built separate binaries because i meant I had to run ./configure less often
< ryanofsky>
If we want unified binaries with a runtime options, that's easy too
< wumpus>
ah yes like busybox
< luke-jr>
eg, bitcoin-qt gets what we have now; bitcoin-qt -process=foo gets just the foo part in MP mode
< wumpus>
that kinda makes sense
< ryanofsky>
these are really just packaging questions, my question is how to make progress on getting code reviewed
< wumpus>
well, 'just packaging questions' are kind of important to do this, I think
< luke-jr>
review club? :P
< wumpus>
if you split up things people are going to ask how to re-bundle them :)
< ryanofsky>
i mean, packaging questions are the things you would be deciding on last, and maybe changing with trial and error
< cfields>
ryanofsky: some of those packaging questions come down to language choices though, those things need to be decided on much earlier.
< ryanofsky>
99% of the code stays the same regardless
< wumpus>
in any case if you want concept ACK on multiprocess, concept ACK, I think it's a good thing in the longer run
< ryanofsky>
cfields ?
< wumpus>
security-wise process isloation is a good start as well
< cfields>
ryanofsky: I'm curious to know, for example, what the downside of using the c++11 version libmultiprocess you've mentioned?
< jonasschnelli>
looking forward to wireguard-tunnel the GUI to a remote node.
< cfields>
does that mean dragging boost back in?
< wumpus>
so maybe the packaging details is the only thing we can disagree on :)
< ryanofsky>
cfields, no downside, i can revert the vasild pr and replace std::optional with boost::optional again
< jonasschnelli>
(but I guess that needs much more)
< wumpus>
wait, why?
< MarcoFalke>
In about 6 months we'll have C++17
< ryanofsky>
jonasschnelli, not too much, you can kind of do it with my existing branch if you use socat (existing branch only create UNIX socket)
< cfields>
wumpus: I'm just trying to understand our options.
< wumpus>
how can we be using std::optional in the first place we haven''t switched to C++17 yet right?
< sipa>
wumpus: libmultiprocess is using std::optional, but we're not using libmultiprocess yet
< ryanofsky>
wumpus, we are not, libmultiprocess used it internally because vasild submitted a pr to get rid of boost, and i thought it was nice
< wumpus>
I think the idea was to have 0.21 still c++11 compatible then go full c++17 for 0.22
< MarcoFalke>
wumpus: libmultiprocess is compiled with c++14 flags in depends
< sipa>
if it's just std::optional or boost::optional... that sounds like something we can just decide whenever it's merged
< wumpus>
c++14??!
< wumpus>
nooo
< MarcoFalke>
s/is/was/
< luke-jr>
do we need to fork upstream to get C++11 support?
< sipa>
if multiprocess integration only lands with 0.22, we'd be fine
< ryanofsky>
it works fine now...
< sipa>
otherwise, it seems it can easily be turned into c++11 compatible
< MarcoFalke>
sipa: Yes, that was my thinking
< wumpus>
I doubt that has to depend on using boost::optional or std::optional they're kind of the same right?
< luke-jr>
ryanofsky: with a non-C++14 compiler?
< wumpus>
could use a wrapper or something like we do for fs.h
< wumpus>
but we intentionally skipped over C++14, we're not going to do that now
< achow101>
don't we have an Optional wrapper already?
< luke-jr>
yes
< wumpus>
achow101: lol yes we do
< ryanofsky>
i'm not sure what's not supposed to work here
< wumpus>
in any case I agree these are all small details?
< MarcoFalke>
I think boost::optional vs std::optional should be the least of our concerns in regard to multiprocess. This is just a sylistic issue
< MarcoFalke>
wumpus: jup
< jonatack>
ryanofsky: why not add the PR to blockers?
< wumpus>
let's go forward with multiprocess imo
< wumpus>
my biggest gripe is really the serialization lib dependency not boost
< wumpus>
everyone wants to get rid of boost but also not get rid of boost it's not going to happen any time soon
< ryanofsky>
jonatack, #16367 has been in high priority list, and was even merged (then got reverted)
< MarcoFalke>
So I think people don't want a half-merged multiprocess? In which case we should focus on the main pr
< wumpus>
we should wrap up the meeting btw
< wumpus>
sorry for only leaving so little time for this
< cfields>
MarcoFalke: I didn't want a half-decided multiprocess. If we've decided to move forward on defaulting it, I have no issue with the merge.
< luke-jr>
it would be nice if there was a standard interface involved here, but lacking that, why not just use serialization.h?
< ryanofsky>
wumpus, it's a valid concern. as much as possible I tried to make framework agnostic to serializtion and allow substituting in other implementations later
< jb55>
wumpus: I've also ran into build issues with changing capnproto versions. is there a plan to rework that branch without capnproto?
< luke-jr>
cfields: why do we need to default it?
< cfields>
luke-jr: ship the binaries by default, sorry.
< luke-jr>
ah
< wumpus>
jb55: so the thing is, I think, that our own serialization framework is not powerful enough
< sipa>
for the wallet<->node communication my big hope was that the protocol would be Bitcoin P2P, so that it can be substituted with whatever on either side... but it doesn't seem anyone's working on that
< jonatack>
ryanofsky: yes; a re-opened PR when it's ready (since you mentioned review)
< wumpus>
jb55: also using consensus serialization for other things might not be the best idea, so people have used something else
< ryanofsky>
sipa, we're maybe getting closer that that with ariard's prs
< sipa>
ryanofsky: okay
< wumpus>
but yes what particular library to use, no idea
< ryanofsky>
he's stripping down the node/wallet interface so it's something more akin to p2p
< MarcoFalke>
sipa: Wouldn't that be dangerous to accidentally connect to a malicious remote p2p node?
< wumpus>
ryanofsky: oh that's nice!
< jb55>
what pr is that
< luke-jr>
just to be clear: we're not planning on making the interfaces backward/forward compatible, right?
< sipa>
MarcoFalke: well you'd have SPV security - which may be even desirable
< sipa>
ryanofsky: (i didn't mean that as a "you shouldn't do what you're doing" - they're orthogonal ways of separating things; a P2P-protocol based one is just what i'm more interested in)
< ryanofsky>
jb55, i think he only has locking PRs open now that make node/wallet more asynhrnous, but he has branches to do things like store block info in wallet i believe
< wumpus>
but for an experimental thing it might be OK to rely on capnproto
< wumpus>
I don't want to stand in the way of that
< ryanofsky>
sipa, yes, I know that, that's the way I look at it too
< wumpus>
we've often enough delayed things before some 'perfect solutoin' would appear which then would never happen
< sipa>
agreed
< jb55>
so experimental with plans to replace it eventually?
< wumpus>
yess
< jb55>
I'm ok with that
< sipa>
ryanofsky: what specifically does capnproto offer that's hard to do in our own serialization.h?
< cfields>
sgtm
< sipa>
or is it just avoiding lots of boilerplate code
< sipa>
(which may be a valid reason too)
< wumpus>
boilerplate I think
< wumpus>
capnproto autogenerates a lot
< ryanofsky>
sipa, it's a whole io framework, more than just serialization
< ryanofsky>
it's higher level even than things like grpc and thrift, it gives you object handles and supports callbacks
< wumpus>
right, it's a real RPC mechanism, not just serialization
< sipa>
i see
< wumpus>
it goes further than protobuf in that regard
< ryanofsky>
and i've found it just very nice to work with, but I've tried to make everything around it as flexible as possible so it could be swapped with something else
< luke-jr>
so might be more apt to compare it to bitcoind's RPC server..
< wumpus>
also it doesn't need to be compatible between differnt verisons
< wumpus>
as it's only used for internal communication
< jb55>
if its in in-tree as an experimental maybe we can get more people working on it... status has been ambiguous for so long
< wumpus>
between components of the same release
< wumpus>
luke-jr: nononono it's *NOT* an official API
< ryanofsky>
wumpus, yes that is true, though capnproto does have same nice versioning / compatibility features as protobufs
< luke-jr>
wumpus: I mean technically comparable, not supported the same
< wumpus>
luke-jr: ok :)
< wumpus>
agreed in that case
< luke-jr>
wumpus: I too would be against a stable interface for this right now :P
< luke-jr>
(unless we literally made it use bitcoind RPC maybe)
< wumpus>
yes, maybe in the future
< wumpus>
ok, let's wrap up the meeting
< cfields>
Thanks for the discussion :)
< wumpus>
thank you too
< ryanofsky>
yes, the interfaces is just the headers in src/interfaces/, changes very often
< sipa>
it's certainly hard to agree on a stable interface 2 major releases before we plan to have it available :p
< wumpus>
sipa: lol exactly
< wumpus>
#endmeeting
< lightningbot>
Meeting ended Thu Apr 16 20:12:09 2020 UTC. Information about MeetBot at http://wiki.debian.org/MeetBot . (v 0.1.4)
< MarcoFalke>
PSA: I will probably clear all travis caches now, because of this bug in the ci scripts ^
< sipa>
i forgot to bring it up again in the meeting, but i'd like some opinions on how to move forward with asmap... 0.20 will have (experimental?) support for it, but the user is responsible for constructing/finding/... the map file somewhere
< sipa>
i have #18573 that adds a binary for constructing/decoding these map files, which is an approach i like, but perhaps it's better to keep this out of bitcoin core's scope for now, unsure
< wumpus>
sipa: I'm not sure, on one hand it's useful to have encoding and decoding in one place, on the other I don't really like adding more and more binaries to the distribution
< wumpus>
MarcoFalke: SGTM
< wumpus>
sipa: we've intentionally kept all kinds of developer tools in bitcoin-maintainer-tools repo instead, but if you think this is something a lot of users are going to need we could ship it with bitcoin core itself sure
< wumpus>
I mean, if everyone is going to generate their own asmap instead of downloading a pre-generated one
< wumpus>
but that's your question I guess
< sipa>
wumpus: or being able to verify a shipped one
< sipa>
but yes, things depend on how these things get used, how distribution is handled, what expectations people have around the process...
< luke-jr>
sipa: ideally we should keep it separate IMO - but does it need to share a lot of code?
< luke-jr>
I hope the multiprocess eventually leads to a separate wallet/GUI repos too ☺
< sipa>
luke-jr: i think the primary advantage is that it allows testing/fuzzing the encoder together with the exact code bitcoin core is using for lookups
< luke-jr>
sipa: no reason we can't test across repos?
< sipa>
it could be a separate repository that's subtreed or otherwise included in the build of course
< sipa>
but that's not really my question
< luke-jr>
nah, just pull it in for QA?
< luke-jr>
like Python
< sipa>
i'm confused
< luke-jr>
sipa: we only need Python to run tests, not to build
< sipa>
sure
< luke-jr>
if bitcoin-asmap is available, we can use it for tests too, but still separate from the main repo
< luke-jr>
no?
< sipa>
luke-jr: you mean if the binary is available?
< luke-jr>
sure
< sipa>
that'd be extremely slow
< luke-jr>
?
< sipa>
if you'd need to invoke the binary for every test case
< sipa>
my fuzz test right now generates random inputs to the encoder function used by bitcoin-asmap directly
< sipa>
that's orders of magnitude faster
< luke-jr>
hmm
< sipa>
and verifies them against the interpreter code used for lookup
< jb55>
sipa: is there a writeup about the asmap stuff somewhere? what's problems its trying to solve, how and why I need to generate those files, etc. as one of the people who maintains the bitcoin package on nixos, would I need to generate that and package it for users, etc?
< sipa>
jb55: we really don't know
< sipa>
it's a binary blob you can load into bitcoin core that affects how it selects outgoing peers and prioritizes incoming connections
< sipa>
that's all there is for 0.20
< sipa>
i have a repo with some python scripts that can generate the file (sipa/asmap), and there is a rust project that can take BGP dump files and produce input for those python scripts
< sipa>
right now it's just an experimental feature as we don't really know what the process for distribution/generation will look like
< sipa>
maybe at some point it becomes part of gitian builds, or something similar
< sipa>
maybe some day it's built into the binary even
< jb55>
it's not much work for package maintainers to pull a signed binary blob from somewhere and include it as an option, or even generate it ourselves from the tools
< sipa>
yes, but who would you trust to sign it
< jb55>
if it was signed with the same key we're using for verifying bitcoin releases for instance
< sipa>
ideally you verify gitian signatures :)
< sipa>
so it's not a single party you're trusting
< sipa>
but if we're talking about the entire process for building these map files... it's not deterministic, as the source material is BGP dumps that come from... somewhere
< sipa>
and they change constantly
< sipa>
so it's not really something that can be gitian-verified
< jb55>
could those be poisoned somehow?
< jb55>
don't know much about bgp
< sipa>
well you need be your own ISP to have access to BGP dumps, and even then you only see "your view" of the internet
< sipa>
you can download BGP dumps from various locations of course
< jb55>
would a typical user want to generate these themselves? how would they verify they are legit? Is the point to prevent eclispe like attacks?
< sipa>
and an attacker supplying you with an asmap file definitely would make eclipse attacks easier for them
< jb55>
right
< sipa>
jb55: better partition resistance, ... too
< sipa>
jb55: i have more questions than answers about the correct procedure :)
< jb55>
could you be attacked during the process of generating the maps? just trying to understand possible attacks here wrt. generating them on the package distro side
< sipa>
so the process is (bgp dumps) -> mapping (currently, using gleb's rust tool); mapping -> asmap file (currently using some python scripts, or the bitcoin-asmap tool i'm adding in 18573)
< sipa>
brb, lunch
< jb55>
sipa: cool I'll take a look
< jonatack>
jb55: there's an extensive review club writeup
< jonatack>
jb55: fwiw i've been running a node with -asmap pointing to the file in a git cloned sipa/asmap repo since last December, you can see the ASN mappings via either getpeerinfo ("mapped_as") or in the GUI peers window ("Mapped AS")
< jb55>
I assume you don't have to worry about this stuff if you run onlynet=onion?
< sipa>
jb55: if you're reachable on public IPv4/IPv6 you may
< sipa>
(onlynet only controls outgoing connections, iirc)
< jb55>
ah
< jonatack>
yes, as per doc/tor.md, incoming connections are not affected by onlynet=onion
< sipa>
to clarify: asmap affects two things independently: the choice of outgoing connections to make, and prioritization of incoming connections when the max connection count is reached
< sipa>
with onlynet=tor the first one disappears of course
< phantomcircuit>
sipa, ip<->asn lookup is definitely something that would be useful outside of bitcoin, i've tried myself to do it in the past but without an active bgp session it's basically impossible to get the map at all
< sipa>
that's good to know
< jonatack>
sipa: that's a good point, and i didn't discuss inbounds in the writeup... may update it
< sipa>
jonatack: i think the outbound selection is certainly the more impactful change
< sipa>
phantomcircuit: though asmap is really about efficiently encoding the map, *once you have a mapping* - i don't think there is a clear-cut optimal solution for building those maps
< sipa>
FWIW, the map with data from asmap-rs (sourced from RIPE NCC data) a week or two ago, with my latest optimized encoder, is 1210027 bytes
< sipa>
however, we certainly don't want to go encourage everyone to go download from RIPE independently... i doubt they're set up for that
< BlueMatt>
note that you probably want to preprocess the ripe ris data feed a bit - it includes a bunch of nonsense paths
< sipa>
maybe we should ask them to publish hashes of their dumps
< wumpus>
I do think this could be useful for other projects, on the other hand, making a library out of it before any other project expressed concrete interest might be scope creep
< sipa>
BlueMatt: i've indeed noticed some bogus stuff in there
< sipa>
fd58:9520:1361:6800::/126 AS136168
< BlueMatt>
heh, thats mild
< BlueMatt>
but, ye
< BlueMatt>
a
< sipa>
BlueMatt: if you have ideas for some useful filtering, please open an issue on https://github.com/0ii/asmap-rs or so
< sipa>
maybe it already does some processing, don't know
< BlueMatt>
hmm, probably a) < /24 /48 v6, b) gotta compare with others, probably
< BlueMatt>
would have to look at differences to figure out whts really broken
< sipa>
another reason for keeping asmap inline for now is that the format is really designed for the specific use case we have (it's only really efficient when you're allowed to remap unused ranges arbitrarily) and isn't super extensible (only AS numbers up to 2^25 i think)
< sipa>
if the codebase is kept in one place it's probably easier to introduce changes
< wumpus>
agree, though 'as part of the bitcoin core project' is in one place, more or less, it doesn't necessarily mean one repository