< bitcoin-git>
bitcoin/master ee11a41 practicalswift: Avoid signed integer overflow when loading a mempool.dat file with a malfo...
< bitcoin-git>
bitcoin/master 027e51f MarcoFalke: Merge #20372: Avoid signed integer overflow when loading a mempool.dat fil...
< bitcoin-git>
[bitcoin] MarcoFalke merged pull request #20372: Avoid signed integer overflow when loading a mempool.dat file with a malformed time field (master...load-mempool-time-integer-overflow) https://github.com/bitcoin/bitcoin/pull/20372
< jonatack>
Sorry to have been reviewing less than usual the past week. After spending time chopping wood on it, #20305 "wallet: introduce fee_rate sat/vB param/option" should be ready for final review for 0.21.
< MarcoFalke>
Fix would be to rebase on current master, but we don't want that
< vasild>
cirrus is not merging the tip of the PR into latest master before testing, like travis?
< MarcoFalke>
not for the config
< vasild>
ok
< MarcoFalke>
Though the code is merged in the merge_base step
< jonasschnelli>
MarcoFalke: as for the devision by 0 in wallet.cpp, ... any idea why cirrus is not triggering the UndefinedBehaviorSanitizer error?
< queip>
recommended nomenclature for keys is: SK - secret key, PK - public key, HPK - hash of PK, right?
< MarcoFalke>
jonasschnelli: Nope. How is your config different from ./ci/test/00_setup_env_native_asan.sh ?
< MarcoFalke>
the ci config is running Ubuntu focal
< jonasschnelli>
I guess clang 8 (bbb) vs 10 (cirrus) should not make a difference
< jonasschnelli>
bbb doesn't set -DARENA_DEBUG
< shesek>
which block validity rules can be validated by spv clients?
< shesek>
the ones I have in mind is checking that the previous block hash connects properly, that the nonce matches the target bits, that the target bits match the last difficulty readjustment, that the difficulty readjustments themselves are valid, and the MTP rule. what more am I missing?
< shesek>
(I got disconnected and didn't see replies if there were any)
< jonasschnelli>
shesek: there where non. :)
< jonasschnelli>
shesek: looks like you have a relatively complete list of what measures you can take when not validating the chain
< pinheadmz>
shesek ive gotten a lot of great help from #bitcoin-core-pr-reviews
< bitcoin-git>
[bitcoin] practicalswift opened pull request #20377: fuzz: Fill various small fuzzing gaps (master...fuzzers-2020-11-12) https://github.com/bitcoin/bitcoin/pull/20377
< shesek>
pinheadmz, is off topic considered okay there when there isn't a meeting going on?
< pinheadmz>
yup, get in there
< shesek>
jonasschnelli, iirc luke-jr ended up finding some issue with his proposed weight fraud proof mechanism, no?
< jonasschnelli>
probably,.. I haven't followed and can't recall
< ja>
luke-jr: writing you here since the thread is already getting really long and i don't wanna add a comment which is answering a question already answered in a posted link
< provoostenator>
(I should try an earlier version just to make sure it's not something else...)
< ja>
provoostenator: is your testing setup public? it would be fun to try and connect all kinds of different versions with each other and draw a matrix :D
< provoostenator>
It connects fine as of a slightly older commit 1d3ec2a1fda7446323786a52da1fd109c01aa6fb
< provoostenator>
ja: it's not unfortunately; you'll have to spin up your own btcd and core machine somewhere.
< luke-jr>
20305 should be noted to break RPC compatibility, but in a way that I and others feel is okay
< luke-jr>
if anyone objects, they should speak up
< luke-jr>
(see details on PR comments)
< wumpus>
versus 0.20? or just versus previous intermediate master releases?
< wumpus>
okay
< luke-jr>
wumpus: 0.20
< jonatack>
(for bumpfee)
< luke-jr>
it's complex to explain, but IMO a reasonable concession
< MarcoFalke>
That certainly needs release notes, no?
< jonatack>
relevant comment by murch on that: "uckily, this is very benign. In the worst case, someone is going to get upped to the minRelayTxFee silently and sends at 1 sat/vB. Since RBF is on by default, they should be able bump when they notice. +1"
< meshcollider>
provoostenator: it's the wrong direction, even if they intended to send at 1btc, they'd end up at 1sat/vB
< michaelfolkson>
Slight overpayment -> Risk of slight monetary loss. Slight underpayment -> Risk of not being propagated? I guess it depends on how high the original fee was
< luke-jr>
minimum realistic bumpfee is probably 2sat/vB?
< meshcollider>
I don't think there's ever any risk of overpayment here
< luke-jr>
which at BTC/kB is 2 BTC/kB, over the absurd fee cutoff
< jonatack>
luke-jr: min incremental fee is 1 sat/vB
< sipa>
luke-jr: good point
< wumpus>
underpayment is at least better, could always bump again
< luke-jr>
so I *think* the magnitude is sufficient to avoid any loss in either direction
< luke-jr>
which only leaves the WU question - but I don't think we have time to do another poll
< luke-jr>
if people prefer WU and have to divide by 4, it's not the end of the world anywaty
< jonatack>
AFAIK sats seem to be far and away what people want
< queip>
ok just was asking
< michaelfolkson>
+1
< luke-jr>
jonatack: well, I assume it'd be sats/WU in that case
< jnewbery>
Once we branch v0.21 (imminently), we'll be able to start using c++14 and c++17 features.
< jnewbery>
I expect lots of people have their favorite features that they want to start using.
< jnewbery>
I was wondering whether we should have a project-wide policy on which new features we should allow, or if there are any we should disallow in the project for now.
< wumpus>
so there's a specific feature that triggers a libc bug?
< jnewbery>
So std::shared_mutex is one that I think we should avoid, at least until we better understand the bug, but are there others?
< MarcoFalke>
std::fs
< wumpus>
I'd rather not do that, that for 0.22 full C++17 is allowed, but that is a good reason to disallow that one for now
< hebasto>
or define minimum libc with fixed bug?
< sipa>
i vaguely remember some issues with trying to use std::filesystem by someone, but i'm not sure if that was because they weren't familiar enough with the build system, or if there were actual problems with it
< MarcoFalke>
We can't use std::fs because LTS versions of operating systems don't ship it
< sipa>
well what's the alternative? does boost::shared_mutex not have the same problem?
< wumpus>
every specific exclusion makes it more difficult for new contributors etc
< MarcoFalke>
sipa: We already use boost::shared_mutex
< sipa>
MarcoFalke: right, my concern is that boost, when compiled in c++17 mode, will just go "oh the STL provides this, just delegate to that"
< wumpus>
people don't generally consult a list of allowed features before contributing
< MarcoFalke>
ah good point
< luke-jr>
hmm
< wumpus>
though things like "use boost::shared_mutex instead of std::shared_mutex" are clear and easy to enforce enough
< wumpus>
or the fs one
< wumpus>
we have our own fs abstraction anyway
< sipa>
it wouldn't be hard to add a linter to outlaw "std::filesistem" and "std::shared_mutex" in the codebase or so
< wumpus>
sipa: yes
< wumpus>
those two are easy
< MarcoFalke>
sipa: The ci would fail if someone tried using std::fs
< sipa>
oh, right
< MarcoFalke>
(and gitian)
< sipa>
my impression is that all these issues are STL features, not languages features
< sipa>
and i expect that if you use a sufficient compiler version, all languages features will actually work
< jonatack>
luke-jr: you are right, feeRate is an option
< sipa>
so perhaps the policy can be "you can use all language features, but initially we'll want to whitelist STL features" ?
< sipa>
that's probably too restrictive
< wumpus>
no,I think it's better to reject specific things if we have a good reason
< sipa>
agree
< wumpus>
and just accept standard C++17 otherwise
< sipa>
i guess the concern is really about things that compile, but are known to not always work
< sipa>
like this shared_mutex thing
< wumpus>
right
< ja>
jnewbery: should std::shared_mutex be avoided even if it is unknown whether boost::shared_mutex is also broken?
< sipa>
and we should just outlaw while necessary... just as with any other compiler/stl bug
< jnewbery>
I don't know enough about compilers to know how stable their C++17 stl features are
< wumpus>
you would hope it is stable after more then three years
< jnewbery>
you would, and yet here we are
< wumpus>
of course, compiler and c++ librar ybugs happen sometimes
< wumpus>
well yes it's a specific thing
< wumpus>
even memcmp had a bug !!!
< jnewbery>
we don't know how many other specific things there are
< wumpus>
should we reject c89 featurs now
< sipa>
haha
< MarcoFalke>
Is there a minimal test case for the shared_mutex thing?
< ja>
it is still not clear to me which glibc bug it actually is, there are linked two different bugs from the c++17 thread
< ja>
but glibc has test cases for the pthread primitives
< wumpus>
it's not possible to guarantee that usage of compiler or library features doesn't have bugs, this is just as true for old features as now ones
< wumpus>
this is one of the biggest risks in consensus driven systems isn't it
< jnewbery>
wumpus: that's doesn't seem like it'd be true. New features are more likely to have bugs
< sipa>
boost 1.71 does not seem to use STL's shared_mutex
< wumpus>
jnewbery: I'm not convinced about that, code rot is a thing
< jnewbery>
*or uncover existing bugs
< wumpus>
new features might actually have *more* eyes on them
< luke-jr>
sipa: unrelease boost might
< wumpus>
this is an abyss without end anyway... we can only calculate in known bugs not potential ones
< luke-jr>
if every distro has a fix, maybe just a sanity check is in order
< MarcoFalke>
A future release of boost might decay boost::shared_mutex into std::shared_mutex
< jnewbery>
My general point is that we shouldn't rush to use new features unless there's very clear benefit
< wumpus>
MarcoFalke: yep
< luke-jr>
otoh, isn't it just a deadlock worst case?
< jnewbery>
and that if we do that, then we should codify what features can be used at the project level
< luke-jr>
I think deadlock is fairly harmless for user-built binaries that simply require an updated OS to fix
< wumpus>
no, I don't think we should make a long list of allowed c++17 features
< ja>
luke-jr: if you use bitcoin to back a lightning node, a deadlock could be pretty bad, no?
< wumpus>
only exclude ones that we know to cause problems
< luke-jr>
ja: hmm, maybe - but aren't there supposed to be watchers?
< wumpus>
we have already made the decision to use c++17, maybe we should be more careful in consensus code, but that's *always* the case for any change
< ja>
luke-jr: i don't know how many people are running watchers. it is irrelevant whether there is supposed to be
< luke-jr>
point is, that would be a scenario with two user errors
< michaelfolkson>
Watchtowers only needed if you aren't online 100 percent of time (or for additional protection)
< wumpus>
and sure, we should take the same care as we did for c++11, don't change things for the sake of changing them
< ja>
michaelfolkson: if you bitcoin node is deadlocked on a shared_mutex, are you online?
< michaelfolkson>
I'd guess not ja :)
< wumpus>
that should also be standard policy for the project already, no needless refactors
< michaelfolkson>
But how long would you be deadlocked for?
< ja>
2 weeks, until you come back from vacation
< jnewbery>
I think only introducing new features as they're needed is the more cautious approach
< luke-jr>
michaelfolkson: indefinitely
< sipa>
michaelfolkson: the definition of a deadlock implies it's forever
< michaelfolkson>
Ok. That is a problem
< MarcoFalke>
jnewbery: I don't think C++17 is "needed". We could stay with C++11 forever
< wumpus>
in new code I think full c++17 should be allowed (apart from what we have labaled as dangerous features)
< luke-jr>
but the same goes for any DoS vuln
< luke-jr>
which seem fairly common
< wumpus>
yes, we could stay in c++11 forever
< wumpus>
but we've already decided not to
< jnewbery>
It would be nice if we could have some nuance in these discussions instead of talking about C89 and staying on C++11 forever
< wumpus>
I don't think it would make the project any safer
< hebasto>
we coudn't stay on C++11 due to new Qt versions
< luke-jr>
hebasto: Qt dropped C++11 support? :o
< wumpus>
just make an exception for the gui code...
< sipa>
waot
< MarcoFalke>
Use C++11 code, but compile it with -std=c++17
< sipa>
this shared_lock thing sounds like a problem in pthread?
< sipa>
which means it would also affect boost?
< wumpus>
boost definitely uses pthread (but maybe in a different way?)
< michaelfolkson>
jnewbery: I think the nuance goes when wumpus says new contributors won't consult guides. But agreed I'd guess nuance is the better way to go
< wumpus>
if it's a general pthread issue then we're already affected and c++17 is unimportant to this
< sipa>
indeed
< jonatack>
At the end of the day, these things are going to come down to code review and incremental adjustments and guidelines as needed.
< sipa>
boost has its own shared_lock implementation
< luke-jr>
hebasto: that looks like the opposite
< sipa>
and doesn't use pthread's rwlock interface
< wumpus>
great
< sipa>
(in 1.71)
< wumpus>
michaelfolkson: I don't think we can front-run any compiler or c++ library issues
< wumpus>
avoiding c++17 features would give a false sense of security imo, it's not like it's brand new
< luke-jr>
I wonder if we ought to add a CI using roconnor's memcmp bug detection
< wumpus>
but that's just my opinion
< luke-jr>
seeing as GCC/distros appear to have a disinterest in actually fixing it
< fanquake>
luke-jr: opposite of what? Qt started using C++14 features in its code, and essentially “forgot” that it was still meant to support c++11. An issue was opened but they never did anything to fix the issue.
< luke-jr>
fanquake: I don't see that mentioning in the linked issue?
< wumpus>
I'm not sure how more nuanced you want this, I don't think it's useful to evaluate every single c++17 feature in the meeting at least
< wumpus>
as if we can judge how much risk the compiler or library change is anyway
< luke-jr>
fanquake: you can't build Qt with C++14 and then link from C++11 code?
< wumpus>
could you ever have predicted this?
< wumpus>
did std::shared_mutex sound dangerous to you than boost::shared_mutex?
< wumpus>
still, here we are
< jnewbery>
changing to something new is always dangerous
< fanquake>
luke-jr: I mentioned the details and linked to upstream in #19305
< wumpus>
okay, so not change to anything new anymore then?
< sipa>
jnewbery: boost has also had its fair share of bugs though
< wumpus>
would this *specifically* have been risky to you?
< sipa>
and a priori it seems a reasonable assumption that things that make it into the C++ standard have matured enough that they're less risky
< wumpus>
if not, it would be a blanket reject c++17 features
< wumpus>
yes
< wumpus>
and yes, boost versions have also broken things sometimes
< wumpus>
not upgrading boost is dangerous too, though
< MarcoFalke>
With other project upgrading, at some point boost::feature is going to be less tested than std::feature. Code changes inside boost, which we can't anticipate are going to eventually break the feautre
< jnewbery>
I don't see any strong arguments against being cautious about new features, but I'm not getting the impression that I've convinced anyone
< wumpus>
I don't think any general principple can be derived from this
< wumpus>
yes, being cautious is always good
< wumpus>
I hope we already are cautious?
< sipa>
jnewbery: i think we should always be caution about new features, but i don't think this is very specific to new language/stl features
< wumpus>
do you have any specific suggestion?
< sipa>
and we do have a review and QA cycle, which are part of the process
< queip>
as it was mentioned, compiling for old mac os x will break, also related to SDK versions afair. In context of Gitian for example
< ja>
the decision should ideally be derived from usage patterns, not from how new the feature is. a 10 year old language feature that nobody uses can't be relied on. the age of the feature is a proxy for how widely used it is, which is a proxy for how buggy it is
< queip>
that is, c++!7 forces such bump of minimal macosx version
< jnewbery>
I didn't think sipa's suggestion of having a list of allowed features was bad. That list would grow over time
< wumpus>
queip: yes, I think that wa calculated in
< wumpus>
I don't think that's a good idea
< wumpus>
it's super confusing to new contributors for example
< michaelfolkson>
Because of new contributors and because of not knowing what should go on that list wumpus?
< sipa>
i think i agree with wumpus now
< wumpus>
yes
< luke-jr>
queip: we typically only guarantee support for the most recent stable version of an OS
< luke-jr>
though Windows is apparently an exception
< sipa>
what constitutes a "feature" even? is a new member function that was added to std::vector a "feature" ?
< wumpus>
if we really dislike certain features then we should disallow them, but I don't think it makes sense to partially allow a standard
< wumpus>
yes
< wumpus>
do we hae to whitelist every function? every class?
< MarcoFalke>
Generally, our insurance against build system bugs are tests
< wumpus>
do you even want to maintain this list?
< sipa>
maybe a good question to ask ourselves: if we had started using std::shared_mutex, would we have caught this issue before release?
< wumpus>
only if we had a test reproducing the issue predictably I think
< MarcoFalke>
we should add one
< jnewbery>
wumpus: I don't think we should be setting our project standards based on what features new contributors might want to use. We have project standards precisely so all contributors write code in a common way
< sipa>
yeah, it'd probably depend on how actively shared_mutex was used (and with how much contention...)
< wumpus>
if it causes a random hang in travis, well, peple would think it's just another infrastructure issue
< wumpus>
jnewbery: okay
< sipa>
jnewbery: that conceptually makes sense, but what specifically is your suggestion?
< wumpus>
jnewbery: feel free to make a list
< wumpus>
jnewbery: and PR it
< wumpus>
jnewbery: I'm not conceptually against it I just do not want to maintain it
< MarcoFalke>
time, btw
< michaelfolkson>
The new contributor argument I don't think is particularly convincing. But the problems of constructing a list is more convincing to me
< luke-jr>
"Travis failed, can someone restart it for me?"
< queip>
apparently not popular opinion, but a white list of allowed free functions, and classes, would make it easier to guard against someone using obscure function that happens to be buggy. Though also a review can anyway guard against it - "why not use the more common solution to this problem". Either way read list of, probably 20 recommended classes and functions, is not that hard for new developer
< wumpus>
queip: again, feel free to maintian such a thing
< jnewbery>
queip: that seems easier to me than arguing on each PR what is and isn't acceptable
< wumpus>
I'm done with this (both the meeting and the discussion)
< sipa>
ok, practically, how would it be decided what is acceptable?
< wumpus>
I think this project becomes impossible to maintian in the close feature
< MarcoFalke>
How could we maintain the list of allowed features?
< wumpus>
if you want to micro-manage every C++ function and class and feature that people use in a huge project like this go ahead
< jonatack>
I don't disagree with guidelines, but reckon they'll be shaped in a bottom-up, rather than top-down, fashion: ongoing, continual, incremental
< jonatack>
(once upon a time, one might have said agile :))))
< ja>
provoostenator: my question is: would it be useful to have a general purpose type : ShouldBeChecked = Check | DontCheck instead of bool?
< ja>
provoostenator: that type could be used for example in the linked commit, and it would alliviate some boolean blindness
< wumpus>
I'm not convinced even CPU features are reliable enough
< sipa>
ja: i see you're a haskell programmer
< wumpus>
if you really want to know
< ja>
provoostenator: it is deliberately ambiguous towards exactly what should be checked. but better than a boolean, no?
< ja>
sipa: i am not advocating lazy evaluation here, mind you ;) i don't think it is an obscure proposal
< MarcoFalke>
Compiling with C++17 will probably make the std lib use those features, and we couldn't protect against that with a allow-list
< wumpus>
I think it's important to isolate the consensus code soon, and be extremely careful about that
< sipa>
jnewbery: so imagine we had this whitelist - at whatever granularity - and someone suggested that std::shared_lock should be added to it... what process would be followed to decide that
< wumpus>
I'm not sure we can be careful enough
< jnewbery>
jonatack: in general, that seems like a reasonable approach. But in this case when there are lots of new features on offer, then starting more restrictive and gradually allowing new features seems more cautious
< sipa>
jnewbery: my guess is... unless we specifically knew about this pthread issue, everyone would be like "that seems very reasonable, it's in every STL, tests work, ... go for it"
< jnewbery>
sipa: I'm not sure. It might not be practical, but I wanted to raise it as something we should at least thing about
< wumpus>
C++ is a way too complex language
< sipa>
wumpus: as are the operating systems and hardware they run on...
< wumpus>
sipa: yes
< sipa>
i don't think that's a cause for despair... we have review and tests
< sipa>
the real world is always messy
< jnewbery>
"This is a bad idea" is different from "I'm not conceptually against it but I don't want to maintain it"
< sipa>
i don't think it's just that
< wumpus>
well...
< sipa>
i don't know how it would be maintained - by anyone
< MarcoFalke>
I don't think an allow-list would conceptually work with c++ features
< wumpus>
it's a bad idea because it takes resources from other things I gues
< wumpus>
it's a huge project
< jonatack>
wumpus: isolating consensus code is a topic i've noticed you come back to repeatedly over time. a good topic for the next coredev...
< wumpus>
an probably not a big thing in risk-reduction in general
< sipa>
jonatack: and every coredev since 2015 or so ;)
< jonatack>
heh
< wumpus>
jonatack: yess
< jnewbery>
wumpus: adding wizzy new c++17 features makes it even huger (conceptually)
< wumpus>
jnewbery: I'm not convinced about that, I don't think there's anythign specific or whizzy about c++17 features
< wumpus>
jnewbery: are there any you *specifcially* take offense to?
< sipa>
jnewbery: maybe... but specifically here, if you didn't know about the pthread thing... wouldn't you think that replacing boost::shared_mutex with std::shared_mutex is reducing risk surface?
< sipa>
happy to see renewed activity on that front with dongcarl's recent PRs
< wumpus>
but I've been screaming into the void about this since forever, I'm happy dongcarl1 picked up on it
< wumpus>
yes
< jnewbery>
ja: I hadn't seen that but my instinct is to agree with practicalswift - it introduces very sharp edges
< jonatack>
jnewbery: in any case for my own work and reviewing, I'll be following your cautious approach
< jnewbery>
new features aren't dangerous just because they may have bugs. They're also dangerous because developers might not know how to use them safely
< sipa>
i think introducing a new features/concepts should always be a red flag for review
< jnewbery>
I trust sipa to use spans correctly, but I wouldn't encourage their use by everyone
< wumpus>
that's different for everyone though, and just as true for old crufty featurs
< wumpus>
(e.g. does anyone know about c++ multiple inheritance specifics?)
< jnewbery>
spans and string_views are objectively more dangeroush than other language features
< michaelfolkson>
We certainly can't have a guide that says if you are sipa you can use it and if you are not you can't haha
< luke-jr>
why are we switching to spans if they're so hard to use?
< wumpus>
jnewbery: well if it's a replacement for void*, size_t
< wumpus>
jnewbery: it isn't any less safe
< sipa>
jnewbery: (playing the devil's advocate here) i think they're less dangerous than points and lengths passed manually
< sipa>
*pointers
< wumpus>
all the span refactores have been that
< wumpus>
just that
< wumpus>
sipa: right
< wumpus>
it makes things conceptually clearer than a pair of pointers and lengths
< wumpus>
that's all, it shouldn't replace owned objects
< bitcoin-git>
[bitcoin] practicalswift opened pull request #20380: doc: Add instructions on how to fuzz the P2P layer using Honggfuzz NetDriver (master...honggfuzz-p2p-fuzzing) https://github.com/bitcoin/bitcoin/pull/20380
< * jonatack>
taking notes on the consensus isolation mentions
< ja>
by using a data-type, it also allows the compiler to understand more invariants about your code
< MarcoFalke>
I think it would be very concerning if people ACK code changes that use features they don't know how to use safely
< ja>
a compiler could have specific protections when using Span, that wouldn't otherwise be used
< MarcoFalke>
*ACKed
< wumpus>
MarcoFalke: yes
< sipa>
ja: about that, can some people please review #19387
< gribble>
https://github.com/bitcoin/bitcoin/issues/19387 | span: update constructors to match c++20 draft spec and add lifetimebound attribute by theuni · Pull Request #19387 · bitcoin/bitcoin · GitHub
< MarcoFalke>
code review is our best effort protecting against bad code
< MarcoFalke>
Developer notes certainly do help to clarify guidelines
< wumpus>
it's good to be cautious anyway, but I don't think making an explicit list of everything is going to help
< wumpus>
and if we're worried about string_view and spans specifically then we should be careful to review changes introducing those carefully
< wumpus>
that's a good point and at least a specific one
< wumpus>
but we can't go over everything and make a risk estimate upfront
< michaelfolkson>
Can I ask about fuzzing? At the moment fuzzing seems to be a two man show (practicalswift and MarcoFalke). Is there anything additional we can do to get more people to engage with fuzzing?
< jnewbery>
I'd be wary of anyone adding try_emplace since the interface is so confusing
< MarcoFalke>
michaelfolkson: You can help by writing fuzzers, reviewing fuzz pull requests, or run the fuzz engine youself and contribute seeds (hopefully ones that don't trigger bugs)
< sipa>
michaelfolkson: i've been adding elaborate fuzzers in several of my recent bigger PRs
< michaelfolkson>
Do you do all your fuzzing on your laptop MarcoFalke? It takes hours sometimes
< wumpus>
michaelfolkson: is there good documentation on how to get started?
< sipa>
i'm less convinced about writing generic fuzzers for code you're not already familiar with
< MarcoFalke>
michaelfolkson: I use my laptop with a fuzz engine, sometimes vim to create seeds. I am also running a server 24/7
< sipa>
it's useful in the long term, but may need very long fuzzing times to hit anything interesting (much less actual issues)
< michaelfolkson>
I think the docs could be improved (but I can't criticise if I haven't tried to improve them)
< MarcoFalke>
michaelfolkson: If there is an issue with the docs you can file a bug (or fix them)
< michaelfolkson>
It is daunting though when they can take hours to run. And there were troubleshooting issues on Mac which I'm going to avoid by running them on Linux instead
< MarcoFalke>
sipa: Agree. The best fuzzers are the ones that trigger the most already-fixed bugs
< michaelfolkson>
Right MarcoFalke. I will try to do this. But lots I don't know currently
< sipa>
michaelfolkson: i run mine on my 16-core desktop, let it run overnight... all actual problems i've found are within minutes/hours of running
< sipa>
(but this is for fuzzers written with very intimite knowledge of what they're testing)
< jonatack>
michaelfolkson: fuzzers aren't an issue to run overnight to review PRs, or even for a few minutes if you're tight on time as a sanity check and describe that in your review
< jonatack>
(i have an older 4-core cpu)
< michaelfolkson>
By a few minutes you mean run them and stop them hours before completion jonatack?
< jonatack>
michaelfolkson: they run until you halt them or they hit an issue
< sipa>
fuzzing never completes
< wumpus>
fuzzers never 'complete' do they
< wumpus>
sipa: heh
< jonatack>
michaelfolkson: when reviewing, sometimes running them for less than a minute finds things, e.g. was the case for the BIP324 implementation
< sipa>
i found several issues while developing txrequest by writing a fuzzer in parallel with it... it found several crashing bugs within minutes
< jonatack>
^
< michaelfolkson>
Does that mean everyone who fuzzes for a few minutes are fuzzing the same thing? And everything else doesn't get "fuzzed"
< sipa>
(all before it was PR'ed, and in some cases, started over)
< sipa>
michaelfolkson: it's making random variations in the seeds, and see if those trigger anything
< sipa>
it's somewhat coverage directed, the fuzzer "learns" what things are potentially relevant and what it isn't
< wumpus>
there's a large degree of randomness involved so no one will be testing exactly the same thing
< sipa>
but it's really just trying random things
< michaelfolkson>
Ah ok. So the randomness ensures different things get "fuzzed"
< sipa>
fuzzing == "introducing random variations"
< sipa>
we do need a bitcoin core specific fuzzing farm that can run fuzzers with months of CPU time though
< jonatack>
there were two really good Review Club meetings on fuzzing
< michaelfolkson>
It certainly seems to me the ultimate goal should be guidance on what will be done on a fuzzing farm and what should be done on individual machines
< sipa>
michaelfolkson: why would there be a difference?
< MarcoFalke>
michaelfolkson: fuzzing should be done
< ja>
michaelfolkson: you could engineer a test that breaks only when path A getting taken exactly a million times, and then path B is taken once. if that is a "different thing", your use of "ensures" is wrong ;)
< MarcoFalke>
It is additive. Running two machines is better than running one machine
< michaelfolkson>
Targeted fuzzing on individual machines. Mass comprehensive fuzzing on a fuzzing farm?
< michaelfolkson>
Would that not be the end goal?
< sipa>
what is "targetted fuzzing" ?
< sipa>
i'll run fuzzers myself locally for code i'm developing if it includes a fuzzer
< michaelfolkson>
" i'm less convinced about writing generic fuzzers for code you're not already familiar with"
< michaelfolkson>
s/targeted/non-generic
< ja>
what's wrong with writing tests for code you don't know? you'll learn the true invariants even if you try to break invariants that don't exist
< sipa>
if we had a server farm, and it's orders of magnitude more compute power than what i can do personally, i wouldn't bother with doing anything else myself
< MarcoFalke>
ja: Nothing wrong, but tests that don't find issues are not that useful
< sipa>
that's not the case though - and all fuzzing right now is people individually running fuzzers
< sipa>
ja: if you do learn, it's a great exercise
< michaelfolkson>
It is a time issue. Only 24 hours in a day
< ja>
MarcoFalke: right, but given that you may find an issue, that could be fixed even if the fuzzing setup doesn't get merged, i think it should be encouraged ;)
< sipa>
michaelfolkson: that was about *writing* fuzzers, not running them
< MarcoFalke>
michaelfolkson: With 2 CPUs there are 48 CPU hours in one day
< michaelfolkson>
Haha ok
< ja>
the bottleneck is always the reviewing, right? but if you find a true invariant getting broken, that is still valuable if the fuzzer doesn't get merged
< MarcoFalke>
ja: Indeed. I am not discouraging anyone to write tests.
< sipa>
my view is that many fuzzers we currently have are very superficial, because they're written by someone who doesn't know what they should be testing
< sipa>
that doesn't mean they're useless, or that i want to discourage people from doing so
< sipa>
but i think there is more value in first understanding the code deeper, and then writing more targetted tests
< michaelfolkson>
The problem is not understanding the Core codebase and history of bugs rather than understanding fuzzing?
< michaelfolkson>
This isn't a "general expertise" in fuzzing issue?
< MarcoFalke>
I do think we have a shortage of expertise
< sipa>
i think there is a useful question to be answered that probably isn't too hard with people unfamiliar with the codebase: can we reasonably merge multiple fuzzers into a single binary (and have the actual fuzzer selected by a command line argument)
< MarcoFalke>
sipa: for the fuzz engine it shouldn't make a difference
< sipa>
we know that it is bad to mix multiple feature tests into one fuzzer (e.g. by using the first N bytes of the input to select the feature), as the fuzz engine will try to cross-pollinate to no avail
< sipa>
but right now the build dir for fuzzing is gigabytes
< MarcoFalke>
the seeds are also gigabytes
< MarcoFalke>
I'll probably create a pull to create a single binary after the branch-off
< MarcoFalke>
then, they can also be compiled normally (by default)
< MarcoFalke>
for non-fuzz builds
< sipa>
yeah, it's not just a space issue, also compile time
< sipa>
running the full linker 100s of times
< MarcoFalke>
why can't ccache cache the linker result?
< sipa>
i'm not sure if linking is ccached, actually
< sipa>
but does it matter? any code change in say net_processing.cpp means all fuzzers have to be fully relinked
< MarcoFalke>
fair enough
< sipa>
MarcoFalke: so you're convinced it doesn't matter for the fuzzing engine?
< MarcoFalke>
why should it?
< sipa>
that sounds reasonable to me, but i thought we'd want to have some kind of benchmark first
< sipa>
more reachable code or something
< sipa>
i don't know exactly how the fuzzing engine reasons about those things
< sipa>
qa-assets directory is 1.3 GiB for me; a full build in fuzz mode is 7.3 GiB
< MarcoFalke>
It might break symbolic execution, but we don't use that
< sipa>
what's symbolic execution?
< MarcoFalke>
Not using values, but symbols (as in math) for detecting violating conditions
< sipa>
ok, i knew that... i mean specifially in the context of fuzzing
< sipa>
are there fuzz engines that can take advantage of that?