< bitcoin-git>
bitcoin/master fab7d95 MarcoFalke: test: Make valgrind.supp work on aarch64
< bitcoin-git>
bitcoin/master 76e6452 fanquake: Merge #19159: test: Make valgrind.supp work on aarch64
< bitcoin-git>
[bitcoin] fanquake merged pull request #19159: test: Make valgrind.supp work on aarch64 (master...2006-valgrindAarch64) https://github.com/bitcoin/bitcoin/pull/19159
< MM77788811>
Are python tests expected to pass in master branch? I build from master and run extended tests and some of them fails.
< fanquake>
MM7778811: They sometimes fail sporadically. If you can't find an issue open with the failure you're seeing, feel free to open one, or, open a PR with a fix.
< hebasto>
something wrong happens with travis arm builds...
< bitcoin-git>
bitcoin/master fa1433a MarcoFalke: rpc: Remove special case for unknown service flags
< bitcoin-git>
bitcoin/master 365f108 Wladimir J. van der Laan: Merge #19112: rpc: Remove special case for unknown service flags
< bitcoin-git>
[bitcoin] laanwj merged pull request #19112: rpc: Remove special case for unknown service flags (master...2005-rpcServiceFlagsUnknown) https://github.com/bitcoin/bitcoin/pull/19112
< bitcoin-git>
bitcoin/master 7daffc6 Gillian Chu: [test] CScriptNum Decode Check as Unit Tests
< bitcoin-git>
bitcoin/master 39afe5b Wladimir J. van der Laan: Merge #19082: test: Moved the CScriptNum asserts into the unit test in scr...
< bitcoin-git>
[bitcoin] laanwj merged pull request #19082: test: Moved the CScriptNum asserts into the unit test in script.py (master...issue24) https://github.com/bitcoin/bitcoin/pull/19082
< phantomcircuit>
wumpus, no i need a permanent secret to set the siphash keys
< phantomcircuit>
if i use a per node siphash key i can avoid recalculating the hashes of all the wallet elements when using the filter index for rescan
< sipa>
phantomcircuit: by per node, you mean one globally for the current node
< sipa>
not one per peer?
< phantomcircuit>
sipa, globally
< phantomcircuit>
im talking about building a new filter index with siphash keys that are random to that node but are fixed
< phantomcircuit>
i don't see how i can add that to the index db itself
< phantomcircuit>
but maybe im missing something obvious
< sipa>
addrman has one
< phantomcircuit>
for benchmarking i just have them fixed as 0 1 but that's kind of easy to guess
< troygiorshev>
If anyone's given it a look I'm happy to answer any questions
< wumpus>
is there a PR or issue for this?
< jeremyrubin>
troygiorshev: I like this kind of stuff. I'm curious if you have intuition on how much overhead it is to track
< wumpus>
if not, I don't think yo ucan expect anyone to have had a look at this
< elichai2>
troygiorshev: the general idea sounds interesting, especially if there will be a defined structure to it, so that external tools can analyze/aggregate the logs
< jamesob>
Concept ACK, would love more comprehensive per-peer statistics for the for-now-fictional monitoring framework I've been noodling on
< sipa>
concept ack, but it's very high level
< troygiorshev>
wumpus: not yet, thanks for the suggestion
< sipa>
things will depend on how invasive it is, what performance impact it has, ...
< sipa>
but in general, gather more of these statistics is definitely useful
< jamesob>
sipa: presumably it would be opt-in, but your questions are still relevant
< wumpus>
also how complex the added code is and how much extra maintence work it is
< troygiorshev>
jeremyrubin: I don't expect it will be run all of the time, but that will be something I keep a close eye on. With it being toggleable, when people are using it they probably won't care about the performace impact (as I don't expect it will be massive my any means)
< sipa>
right, but even if it's opt-in, if it's so invasive that it alters the data it's measuring... it may be a problem :)
< jamesob>
true!
< troygiorshev>
elichai2: absolutely. I'll also be building a bit of an analysis tool
< jamesob>
maybe there's a validation-queue-style decoupling you could do (or resurrect the logging thread PR and extend that)
< jnewbery>
jamesob: for stats tracking it might not be opt-in. Eventually we might want to use those stats to do peer throttling/eviction
< wumpus>
it reminds me of the statoshi changes (which IIRC never got integrated, nor even proposed, beause they're too invasive)
< sipa>
jnewbery: indeed
< phantomcircuit>
hi
< jamesob>
jnewbery: makes sense, but could start opt-in and make various measurements mandatory on a case-by-case basis
< sipa>
of course
< jonatack>
troygiorshev: i read it. i think it addresses a real issue but needs much more detail and also describe much more any previous work on this
< luke-jr>
one thing to keep in mind is we don't want detailed logging by default
< jamesob>
wumpus: and they introduce a dependency on a statsd library iirc
< jnewbery>
wumpus: statoshi is global statistics for the node. This is more targetted towards troubleshooting/investigating interactions with individual peers
< wumpus>
in which case you might want to talk with Jameson Lopp
< luke-jr>
last thing we need is people getting subpoena'd to find out which peer relayed what
< wumpus>
jnewbery: which sounds even more involved
< sipa>
luke-jr: logging to an external file should definitely remain opt-in, of course
< wumpus>
luke-jr: that's also a serious risk
< wumpus>
detailed logging should *definitely* not be enabled by default
< jamesob>
is there a near costless way of "teeing" each P2P message received to some separate analysis thread?
< troygiorshev>
jamesob, sipa: I hope that, in its initial form, it will have no impact whatsover when disabled, and that it won't modify messages in any way when enabled. imo it wouldn't be an effetive monitoring tool if it changed things
< troygiorshev>
(We're not quanum physics here :))
< sipa>
sure
< jamesob>
troygiorshev: I think the concern is not message modification but introducing some delay that materially affects p2p behavior as a second-order thing
< wumpus>
jamesob: tcpdump? :)
< jeremyrubin>
TBH I think the best thing is tcpdump-like
< jamesob>
wumpus: sounds good to me! let's not do this in core if that's practical!
< sipa>
i think there are two separate goals, and they may need different solutions
< jeremyrubin>
You might want to do it in core for encryption reasons
< wumpus>
wireshark has a bitcoin P2P protocol dissector, for example
< sipa>
dumping logs may be doable with tcpdump or other external tools
< jnewbery>
I don't think there's a huge amount to discuss yet. I just suggested that troy raise this in a meeting so people are aware of the project and know where to leave feedback/suggestions
< sipa>
but tracking of statistics that we may want to one day use to influence p2p behavior can't be done that way
< wumpus>
if your goal is really to capture all P2P data, that approach sounds better than adding yet another layer in bitcoind
< MarcoFalke>
I think it is hard to give feedback when the exact goal is a bit vague right now. Is this for internal node stats? Is this for logging plaintext to a file? Is this for exporting stats to a serialized file?
< wumpus>
yes it's useless for statistics
< wumpus>
agree there's mixed goals here
< jamesob>
an OOB process could then feed back into core via an RPC interface, if desired
< wumpus>
we already have some light statistics, like a P2P message type histogram in core, doesn't hurt to add more oft hose things esp. if they're opt-in and don't add to memory use of the CNode struct too much
< phantomcircuit>
wireguard even has a dissector for this
< sipa>
yeah
< wumpus>
but if you want full traffic intercept, that kind of thing doesn't belong in bitcoind imo
< wumpus>
phantomcircuit: yup
< jamesob>
troygiorshev: anyway, looking forward to some details!
< jnewbery>
wumpus: tcpdump isn't aware of application details. It'd be impossible to log messages from peers with subversion="thing" for example
< jnewbery>
unless you dumped all traffic all the time and then filtered after the fact
< troygiorshev>
lots of good points, thanks. Certainly I agree, it's not for statistics. The immediate benefit would be for debugging, one of the long term benefits could be intelligent peer-selection
< sipa>
yeah, let's wait for some details
< jb55>
troygiorshev: I have been adding usdt bpf traces to my node for exactly this use case. it adds low-overhead nop instructions that linux can plug into at runtime. and with bpftrace you can write scripts to gather node stats
< luke-jr>
not impossible w wireshark
< phantomcircuit>
jnewbery, the wireguard dissector actually means you can do exactly that
< luke-jr>
though maybe not implemented
< wumpus>
jnewbery: dunno about tcpdump but wireshark can do some quite complex context/application-specific filters
< jnewbery>
yes, I understand you can filter afterwards. The point is that you end up with enormous pcap files because you have to capture all traffic
< wumpus>
yes, but also it is a pretty rare case for most people
< wumpus>
some things should really be external tooling
< luke-jr>
phantomcircuit: that just matches the version packet tho?
< phantomcircuit>
luke-jr, you can select tcp streams based on it though
< wumpus>
yupp
< phantomcircuit>
i cant remember how but i know you can
< sipa>
fwiw, i'm more interested in gathering statistics (possibly some day gathered by default) than just protocol dumps
< jonasschnelli>
would ne no longer fun for post BIP324 connections
< jonasschnelli>
(outside core)
< sipa>
jonasschnelli: also a good point
< jamesob>
would be nice to make it easy for people to voluntarily give a detailed dump of p2p stats
< luke-jr>
cool
< troygiorshev>
i'm personally a huge fan of BIP324 and AltNet and related, so I'
< troygiorshev>
I'm keeping those in the back of my mind as I think about htis
< wumpus>
so: statistics yes, protocol dump controversial
< jnewbery>
right, it's impossible to do any kind of dynamic packet filtering outside the application if it's encrypted
< wumpus>
let's regard thes as separate topics
< luke-jr>
jnewbery: NOT IF YOU PROVIDE KEYS
< luke-jr>
oops sorry for caps
< sipa>
shower thought: are pcap files easy? if so, maybe it's useful post-BIP324 to permit dumping the decrypted stream in pcap format
< jonasschnelli>
luke-jr: would probably be complicated to impossible in wireshark
< wumpus>
but I'd suggest first writing some document detailing more what you want to do, or make a example implementation
< luke-jr>
jonasschnelli: it does for https already
< MarcoFalke>
For logging, one could disable bip324
< sipa>
indeed, seems there are many partially overlapping ideas here
< wumpus>
as said this is quite vague and abstract right now
< jnewbery>
sipa: I don't think you'd want pcap. By the time you've got to the application, you've lost all the lower protocol layers
< sipa>
perhaps
< wumpus>
you could add these zero-cost linux abstraction hooks to bitcoind (forgot the name)
< jb55>
ebpf
< wumpus>
yess
< jb55>
I have a branch for it
< wumpus>
that's extremely flexible and allows for all kinds of diagnostics and experiments without impact on the code itself
< jb55>
I've been using it to time ibd and gathering histograms. it just inserts NOP instructions that linux hooks into
< jb55>
so only had overhead when you are plugged in
< jb55>
but you could also just compile it with traces disabled
< troygiorshev>
well I'll definitely be checking out everything that was said here, thanks everyone!
< wumpus>
someone even did a presentation at a bitcoin coredev meetup about this once, I forgot who
< luke-jr>
iirc gcc can inject tracing stuff auto
< jb55>
here's an example bpftrace script that I am using + a connect_block usdt tracepoint https://jb55.com/s/ibd.bt.txt
< wumpus>
jb55: thanks!
< sipa>
seeing USDT in this channel scares me
< troygiorshev>
jb55: amazing!
< jb55>
so you can share these bpftrace scripts that gather stats without hardcoding anything into core itself, you just have to insert the tracepoints in interesting places
< jb55>
and you can extract values as well from the tracepoints, so you can log structs, etc
< jnewbery>
wumpus: are you thinking of evan klitzke's work on flame graphs and probing?
< wumpus>
eklitzke was that
< jamesob>
jnewbery: I thought same but I think those are kernel probs; this looks like userland stuff
< wumpus>
yes
< jamesob>
*probes
< wumpus>
they work in userland too
< wumpus>
jnewbery: yes!
< jb55>
yeah the perf tool uses the same underlying tech, linux tracepoints, ebpf a way of executing these scripts securely within the kernel as bytecode
< wumpus>
could add a probe that is called on every P2P packet, for ex.
< jb55>
that's what the script I linked is doing. that is compiled and run within the kernel, and taps into those usdt tracepoints
< jb55>
it can also access low level io/net kernel traces within the same script which is nice
< jb55>
might be overkill but its super powerful
< wumpus>
that's really awesome
< wumpus>
well it's overkill but also the range of things you miht need for diagnosing or testing specific things calls for something flexible which is why it is that way
< wumpus>
the alternative is generally to recompile with all kinds of instrumentation but that requires a recompile and patching every time
< jamesob>
seems like it'd make a lot of sense to build in ./configure-able dtrace taps...
< jb55>
if there's interest I would PR it, just wasn't sure
< wumpus>
there definitely is interest!
< jb55>
kk
< jonatack>
jb55: yes
< jamesob>
jb55: cool work
< sipa>
indeed
< wumpus>
we need to cover some other topics today so moving on for now, maybe discuss this further out of meeting
< wumpus>
#topic Merging of Schnorr in libsecp256k1 (sipa)
< sipa>
hi
< troygiorshev>
yep thanks again everyone I'll make an issue with this more formalized and many questions answered!
< wumpus>
troygiorshev: thanks
< sipa>
so with the prospect of having BIP340-342 one day merged, there will need to be a time to merge BIP340 support in libsecp256k1
< sipa>
and i was wondering if we have a good feel for when the right time is
< wumpus>
I don't see any drawbacks to merging it as an optionally enabled feature
< sipa>
yeah
< sipa>
i think the code is pretty much done; it was recently stripped of some future feature (batch validation) and cleaned up
< nickler>
it's an experimental module right now, so it needs to be explicitly enabled
< sipa>
of course it'd be experimental for now
< sipa>
(thanks to nickler)
< sipa>
do we want to update libsecp256k1 in master before adding the schnorr code first?
< sipa>
so that the diff is minimized?
< wumpus>
I guess before exposing it publicly there needs to be some agreement with regard to the interface, though not 100%, it's expected for there to be some drift over the initial releases with it
< wumpus>
sipa: I don't think that can hurt in any case
< sipa>
to me, it feels that there needs to be some confidence that this is the functionality that will eventually make it into bitcoin
< wumpus>
last secp256k1 subtree update has been a while
< sipa>
we wouldn't just merge support for anything in libsecp256k1, being a subproject of bitcoin core
< wumpus>
there's a lot of expectations around it ending up in bitcoin eventually at least
< sipa>
but i think that confidence can be (significantly) lower than full consensus rules implemented in core
< sipa>
*than what is needed for
< nickler>
fwiw the PR looks good to me right now and many people looked at it already but it has changed quite a bit over time. If these people would be interested in having another look, that would be helpful.
< elichai2>
I'd like to review the new keypair direction but I do hope that BIP340 will actually end up in bitcoin in the end so I think it's ok to move toward merging schnorr to libsecp
< wumpus>
I think the most important thing to be confident about is that the algorithm, and the code, is secure
< real_or_random>
elichai2: I mean we don't need to merge it in the current state.
< real_or_random>
but it's good to know when we feel confident enough that we want it in the codebase at all
< sipa>
yeah
< sipa>
that's why i'm bringing it up here
< sipa>
unless there are other comments, that's all from me
< nickler>
Afaik we've addressed all comments on the BIPS on the mailing list in some form of another
< sipa>
some TODOs left for clarifications/rationales, but no semantics changes
< jamesob>
who are the people who have spent enough time on this to give meaningful ACKs?
< nickler>
for the libsecp PR there's a few that could review it relatively quickly (elichai, real-or-random, sipa)
< jamesob>
I'm curious if there's anyone here who *doesn't* think schnorr will or should eventually be a part of bitcoin
< elichai2>
I think the big "political" question in terms of merging is if anyone believes that BIP340 doesn't have a good chance of landing in bitcoin core
< sipa>
jamesob: i think the right question is whether it will be part of bitcoin *in this form*
< jamesob>
sipa: ah
< troygiorshev>
wumpus: thanks
< elichai2>
sipa: by *this form* I assume you mean the algorithm and not the API?
< sipa>
elichai2: yes
< elichai2>
sipa: did you get any comments on the curve mailinglist?
< wumpus>
we have 10 minutes left and two (small) topics left
< jamesob>
I think anyone who feels they're capable of evaluation should speak up, because I think a whole lot of us are not qualified...
< real_or_random>
also not everything is in the current PR
< real_or_random>
e.g., no batch verification at the moment
< wumpus>
but I guess it's better to postpone those to next week
< real_or_random>
we (the BIPauthors) feel it's in a good state, I think otherwise sipa wouldn't bring this up
< elichai2>
The last time I reviewed it it was in a pretty good state, I assume it's even better now
< jeremyrubin>
jamesob: it's also useful to have people out themselves as useless
< jamesob>
jeremyrubin: /me raises hand
< jeremyrubin>
jamesob: rather than seeming passive ack. I'm not opposed but am unqualified to comment
< jamesob>
I think I would not be wrong in characterizing most people at this meeting as "generally favorable towards schnorr -> bitcoin but unable to provide meaningful commentary on the specifics"
< sipa>
i think part of the question is here are about measuring community expectations, and not necessarily technical review itself
< jamesob>
IMO PR it. Best way to get the qualified commentary out.
< wumpus>
definitely favorable enough to merge it into secp256k1, which is a small step and easily reverted
< MarcoFalke>
Well, a bump to the current libsecp wouldn't hurt
< MarcoFalke>
The overhead is just another two commits
< sipa>
MarcoFalke: I'll PR a bump (pre-schnorr) soon
< MarcoFalke>
thx
< sipa>
there have been some nice improvements in master too
< wumpus>
I don't think this is a point where Schnorr progress should be held up on
< wumpus>
ofc. there will be a long stuggle to get it into bitcoin itself and hopefully a lot of people that will review the cryptography and code in detail
< MarcoFalke>
oh, no. There was 0.16.3, so it was released after all
< gwillen>
I am surprised not to have seen much discussion of this yet
< MarcoFalke>
gwillen: Hasn't this been discussed on the mailing list a while ago?
< achow101>
gwillen: it was ostensibly public knowledge already, we just happened to be ignoring it
< gwillen>
ok, makes sense
< MarcoFalke>
Pretty sure I saw at least one thread with this (or somthing like that)
< achow101>
but with hardware wallets doing something about it, we will need to change PSBT stuff
< achow101>
which I'm working on
< sipa>
i assumed that everyone thought it was too hard to deal with, and only of minimal impact (attackers would need to consistently get you to sign twice, and only gain miners money)
< sipa>
it's fixed (and referenced) in BIP341
< gwillen>
right, they would need either a colluding miner, or they would have to make it an extortion threat ("pay me or I burn your coins")
< achow101>
I guess there's a question if we want the Core signer to also require non_witness_utxo too for segwit inputs and basically just ignore witness_utxo
< achow101>
or maybe supplement witness_utxo with utxo set info. or just do nothing
< achow101>
i'm inclined to go with do nothing
< sipa>
that'll break the ability to sign on trezor (and others that adopt similar changes)?
< achow101>
and just change updating to also put a non_witnes_utxo for segwit inputs
< gwillen>
I don't think that requiring the full UTXO actually fixes the whole problem, although it makes it harder to exploit
< sipa>
gwillen: if signers verify that information, it's a full solution i think
< achow101>
sipa: we only need to change updating to let signing work
< achow101>
with those hardware wallets
< sipa>
yes
< sipa>
because i think our signer already verifies the full non-witness utxo information if it's present
< sipa>
right?
< achow101>
yes
< achow101>
what I mean is whether we should have our signer require non_witness_utxo for segwit inputs too as the hardware wallets are doing
< sipa>
that's a good question
< sipa>
perhaps it should - but have the ability to disable it?
< gwillen>
sipa: nevermind, the alternate attack I was imagining doesn't work
< achow101>
I was thinking possibly look up the UTXO in the UTXO set if we have a witness_utxo
< achow101>
otherwise require the non_witness_utxo
< sipa>
achow101: that also works, but makes the signer non-stateless
< sipa>
*stateful
< achow101>
yes..
< sipa>
but it is a great solution
< achow101>
also means I won't have to rewrite all of the psbt tests
< sipa>
though, why are you signing on an online machine? :p
< achow101>
because you're running the functional tests :)
< sipa>
ha
< achow101>
maybe something to discuss at the wallet meeting tomorrow?
< sipa>
yeah
< phantomcircuit>
i could do something horrible and stuff the nonce into the 32 bits of zero in the block hash
< jeremyrubin>
phantomcircuit: you can also just elide the block hash entirely and get 32 bytes of space
< sipa>
phantomcircuit: why do you need a per-node salt? given that the gcs code tweaks by block hash, it should be unpredictable to anyone even with a fixed siphash key
< phantomcircuit>
sipa, so that it doesn't tweak by block hash and i dont have to recalculate the siphash of every script pub key for every block
< sipa>
ah of course
< phantomcircuit>
it kind of all strongly assumes you dont want to do that though so it's kind of annoying
< phantomcircuit>
thus the hack
< bitcoin-git>
[bitcoin] PastaPastaPasta opened pull request #19169: rpc: Validate provided keys for query_options parameter in listunspent (master...bitcoin-validate-keys-listunspent) https://github.com/bitcoin/bitcoin/pull/19169
< bitcoin-git>
[bitcoin] luke-jr opened pull request #19170: [0.20] Add missing QPainterPath include (0.20...bugfix_incl_qpainterpath-0.9) https://github.com/bitcoin/bitcoin/pull/19170