< bitcoin-git>
[bitcoin] jnewbery opened pull request #19085: Refactor: clean up PeriodicFlush() (master...2020-05-refactor-periodic-flush) https://github.com/bitcoin/bitcoin/pull/19085
< jeremyrubin>
For #18191 I'm really conflicted with what to do on the compiler warnings that jonatack is reporting. The const txiter that I wrote is intentionally const. I'm a const-me-if-you-can beleiver. But it causes a false-positive compiler warning on certain clangs, and I don't love removing a safety annotation for a compiler warning.
< jeremyrubin>
I'm happy to do whatever wumpus says I should do here
< sipa>
jeremyrubin: making it const txiter& isn't removing any safety?
< jeremyrubin>
Ah right I am a bit foggy today I misremembered what the fix is and why I didn't like it. I think maybe if we should change this code, it's maybe worth style guiding to not use copy vs ref or remove -Wrange-loop-analysis if we don't want to require it
< sipa>
i think for any moderately decent optimizing compiler, passing by reference is never worse (and often better)
< sipa>
so the advice of -Wrange-loop-analysis always applies
< jeremyrubin>
ok I guess i'll squash in changing them. My frustration is less so about this one in specific and more so that there's like 30 some odd other commits that I need to re-write for this warning for the pending work on the mempool project.
< jeremyrubin>
Is there any way that gcc can throw this warning as well or we can make it more reliably triggered?
< jeremyrubin>
I re-asked this on the github thread in case anyone knows
< bitcoin-git>
[bitcoin] fanquake opened pull request #19088: validation: use std::chrono throughout some validation functions (master...validation_chrono) https://github.com/bitcoin/bitcoin/pull/19088
< hebasto>
MarcoFalke: recent changes in #19064 introduced new circular deps. Which is the best: revert changes or allow new circular deps?
< wumpus>
hebasto: as the goal of the PR is only 'readabillity and maintainability' I'd err on the site of reverting the part that introduces the circular dependency
< wumpus>
if it was anything critical like a bugfix or feeature I'd say otherwise
< hebasto>
wumpus: thanks
< wumpus>
in general we have checks like 'prevent circular dependencies' because we want to work toward a state where there are none, ideally all commits should leave the source in a better state in that regard than they found is
< bitcoin-git>
[bitcoin] laanwj opened pull request #19094: build: Only allow ASCII identifiers (master...2020_05_no_extended_identifiers) https://github.com/bitcoin/bitcoin/pull/19094
< bitcoin-git>
[bitcoin] jnewbery opened pull request #19095: [tools] Update clang-format config for multi-line function declarations and calls (master...2020-05-clang-tidy) https://github.com/bitcoin/bitcoin/pull/19095
< provoostenator>
For code review, but if it gets stuck in another concept discussion, we can bump it to that column.
< wumpus>
achow101, sipa, fjahr: added
< wumpus>
sipa: yess finally
< sipa>
thanks
< fjahr>
Thank you!
< MarcoFalke>
sipa: Was good that they were split up into reviewable chunks
< wumpus>
provoostenator: added
< wumpus>
right, that really helped
< wumpus>
promag: added
< sipa>
MarcoFalke: yes, it really helped; the resulting changes were much better than the original monolithic PR too
< MarcoFalke>
#18971 has almost 20 commits and changes 1000 lines of code. that sounds like a whole afternoon of review. I wonder if it can be split up as well
< provoostenator>
Same, it seems worth a try and easy to reverse.
< MarcoFalke>
It is hard to predict if that is going to be benefical for the GUI (Does it increase review or decrease?)
< jnewbery>
concept ACK. Haven't thought too much about the approach
< achow101>
i feel like that might kill the gui further..
< wumpus>
I'd like it to increase contributions mainly
< MarcoFalke>
Yes, the change is only "meta" (no build system changes), so it should be trivial to revert
< wumpus>
currently it's *scary* to contribute to the GUI being part of the same repository as the consensus code
< MarcoFalke>
wumpus: Same, and I think it will.
< wumpus>
this turns away some ppl
< jonasschnelli>
Yeah. Definitively worth a try.
< jamesob>
wumpus: do you really think it actually turns away people, just that GUI is under bitcoin/bitcoin?
< wumpus>
jamesob: yes
< MarcoFalke>
The GUI review isn't in the best state anyway right now. It can't get much worse tbh
< wumpus>
the bar is really high
< provoostenator>
It's hard to predict. The smaller Github repo might create more of a critical mass for GUI devs. Or it slows down. But in that case we can undo.
< jamesob>
wumpus: but won't process be the same in the GUI "repo"?
< wumpus>
MarcoFalke: also true, we could get some more people involed then too
< jonatack>
I think the key question is if it will draw new contributors to it.
< wumpus>
jamesob: the process, yeah, I guess, but we don't have to have exactly the same team there
< MarcoFalke>
jonatack: Even if it didn't draw any new contributors, but the existing ones can more effectively work on core or the gui (or both), then that is already a win, imo
< promag>
but in what way does it ease new comers and progress if it's another repo? and at the end its all built together?
< jamesob>
afaict all this change (as proposed) accomplishes is segmentation of email/github alerts & issue/PR queues - and I'm certainly not knocking that
< wumpus>
promag: marcofalke's work is one step
< MarcoFalke>
jamesob: Yes, it is mostly a meta way to form different notification streams
< wumpus>
I think eventually the GUI should evolve faster / partially separate from the rest
< jonasschnelli>
The PR/issue separation is IMO already solvable
< promag>
isn't it better to do this after multiprocess is in?
< wumpus>
but even separating things like issues is probably good
< wumpus>
promag: it doesn't matter
< provoostenator>
I've _rarely_ used the GUI tag to look for GUI PR's. I don't know if I'm more likely to look at a seperate repo, but I image that I'm reviewing 1 GUI PR, I'm more likely to notice another one.
< wumpus>
the current repository just has too wide a scope
< wumpus>
it makes sense, conceptually, in the long term to separate things out so why not try to make a little progress
< MarcoFalke>
It is not only about the tag, but any kind of communication or notifications
< harding>
I think it's important for Bitcoin Core to continue to ship releases with a default GUI, which this allows, and making it easier for people to follow just GUI issues sounds very nice to me, so +1
< jnewbery>
promag: there aren't any dependencies between separating GUI into a different repo and multiprocess
< troygiorshev>
i can imagine it may help attract people who are more UI/UX focused, and who would be scared away by the breadth of the main repo
< wumpus>
harding: yeah what we ship is not going to change
< harding>
wumpus: excellent! I was rather worried about that when I saw the proposed meeting topic.
< gwillen>
jnewbery: I was going to kind of ask about the same thing, I imagine that a clean interface separation would make it much easier for people to work on the GUI with confidence about not touching anything in consensus or whatever
< jamesob>
troygiorshev: per this proposal, the breadth in terms of code will be the same (which I think may confuse people)
< wumpus>
troygiorshev: right
< MarcoFalke>
I think the only requirement for this split was the src/interfaces cleanup, because previously the gui directly accessed node globals and state IIRC
< wumpus>
gwillen: there is a clear interface separation already
< jnewbery>
gwillen: there already is clean interface separation
< wumpus>
has been there, for a while
< achow101>
I think this will make some wallet improvements harder. the gui almost has direct access to wallet things and some wallet changes have an effect on the gui
< gwillen>
I guess a lot of GUI changes do respect that, it's my own fault that my own GUI changes also require changes to that interface and so touch both sides
< achow101>
so then you end up with having a wallet change that requires simultaneous gui changes. with 2 repos, that's more difficult
< jnewbery>
at the moment, both sides of that interface are in the same process. But everything goes through src/interfaces/node
< promag>
but whats the idea? someone clones the gui repo, pulls core somehow and builds eveything?
< gwillen>
one thing I see a lot of is the GUI reaching "through" the interface layer to grab a direct pointer to an object on the other side and twiddle it
< gwillen>
but anyway maybe this is off the current topic
< wumpus>
achow101: for most things the GUI shouldn't care about the *kind* of wallet though
< jnewbery>
that interface was introduced a couple of years ago and has been cleaned up continually. If separating the GUI dev process into a separate repo makes us clean it up faster, so much the better!
< wumpus>
achow101: e.g. imagine the GUI or some other software accesses the core wallet through RPC, why would it care the wallet was implemented differently?
< MarcoFalke>
achow101: If a wallet change has an effect on the gui (e.g. a wallet method was renamed), then that simply goes into the main repo
< sipa>
i don't have much of an opinion on the repo separation here... i'm skeptical that it will help, but i agree it's easy to revert if not
< wumpus>
achow101: seems also a matter of interface design
< jamesob>
it sounds like some people are conflating this proposal with having separate source trees in separate repos; the proposal as-is (IIUC) is to have the same source tree in two separate github repos just to segment github workflow
< promag>
adding stuff to core+gui at the same time will take longer too right?
< wumpus>
promag: yes
< wumpus>
promag: but the preferred flow *already* is, and has been for a long time: implement it in bitcoind, then later add it to the GUI
< MarcoFalke>
I also don't think we will see groundbreaking changes, but at least we can gather some data points and experience. And based on those a future "complete" split will be easier to reject or accept.
< sipa>
as for better defined interfaces... if the hope is that this will result in more GUI work, and that happens, I expect we'll find out that more work on the GUI will entail more changes to the interface as well... and having things in separate repositories will only complicate things (i realize that this is not what this PR does, but if that's the eventual goal... it can cut both ways)
< jamesob>
sipa: agreed
< jnewbery>
sipa: sounds like a good problem
< wumpus>
I like to split up the repository, ideally I'd like to have started at seperating out consensus code, but we all agree it's much harder than the GUI :)
< achow101>
wumpus: I think a specific example of what I'm talking about was with descriptor wallets and watchonly. The GUI had to display different things for descriptor wallets because of the different watchonly behavior, so there needed to be simultaneous wallet and gui changes otherwise the gui would show the wrong thing.
< MarcoFalke>
sipa: It is currently not decided if interfaces count to the gui side or the node side
< achow101>
I suppose that's more an artifact of legacy wallets though and maybe doesn't matter moving forwards
< sipa>
MarcoFalke: node side, obviously?
< jonasschnelli>
why would the interface be on the GUI side?
< wumpus>
achow101: I agree it will make some things more difficult, though, that seems like a one-time thing
< sipa>
they're also used by RPC, no?
< MarcoFalke>
are they?
< wumpus>
the interface would be node-side, I guess
< MarcoFalke>
the rpc directly calls into the node right now
< wumpus>
that's the idea of an interface
< jonasschnelli>
it's an interface,.. the GUI consumes/adapts to it.
< wumpus>
yes
< wumpus>
the node defines the interface the GUI uses it
< jnewbery>
the rpc doesn't use the interface (but I agree with everyone else that it's part of the node)
< sipa>
huh
< wumpus>
the GUI can have arbitrary changes as long as the interface doesn't need to change
< jonasschnelli>
and since the interface is on the node/core side, I think changing the GUI will be much harder
< wumpus>
no, the RPC doesn't use that interface
< wumpus>
that doesn't matter here though
< sipa>
ok, i never paid attention to the interface side, but i assumed it would be shared between GUI and RPC
< wumpus>
RPC is very tightly bound to the node and that's not going to change any time soon
< jamesob>
ironic that the guy leading process sep (ryanofsky) isn't saying anything!
< MarcoFalke>
silence means ACK, right?
< jamesob>
maxim of the law, yes
< jonatack>
What were the pain points driving the proposed change? ISTM this isn't clear in the RFC. Lack of GUI review? Other things?
< troygiorshev>
is there confusion in where PRs that touch both sides would go?
< jnewbery>
but again, process sep is almost completely orthogonal
< wumpus>
jonasschnelli: it will make changing the GUI harder *if* it needs an interface change
< provoostenator>
Two seperate repos might also make it (slightly) easier to demo more radical forms of splitting the code.
< jamesob>
jnewbery: right
< jonasschnelli>
MarcoFalke: I'm currently working on a GUI PR that (mempool histogram) that changes the interface.. how would I have to proceed?
< wumpus>
jonasschnelli: if it's internal to the GUI, say, moving around some buttons or changing dialogs, not so much, and that's the kind of thing that *needs* to be easier
< provoostenator>
troygiorshev: no, they go to the main repo
< jonasschnelli>
First PR the interface change (without an consuming element),... then PR the GUI part? Or simultanously... how do we handle the merge?
< promag>
wumpus: why? is it hard?
< provoostenator>
jonasschnelli: I usually make two PR's that build on eachother
< MarcoFalke>
jonasschnelli: Well, everyone except me said that interfaces are node code, so I guess you will have to add the interface first and then make the gui changes
< sipa>
jnewbery: i don't know if an outcome where it's easier to make nitty changes, but harder to make substantial changes, is an improvement
< wumpus>
yes, you'll have to change the interface first
< wumpus>
same as if you were going to change an RPC-facing application and needed some new interface
< jonasschnelli>
MarcoFalke: what would be merged first? Or simulatnously?
< MarcoFalke>
Obviously this means there will be an unsused method in the interface temporarily, but that should be ok
< gwillen>
sipa: well, I think it depends on what your goal is for the GUI
< MarcoFalke>
the interface change will be merged first
< gwillen>
right now it is clearly a user interface designed by programmers who would rather be doing literally anything other than designing a user interface ;-)
< jonasschnelli>
what is the GUI change never gets merged?
< MarcoFalke>
It can also be merged at the same time, I guess?
< gwillen>
I assume the hope is that separating it means that the GUI can get a lot more work from people who have more expertise in GUIs and less in the backend behind it
< jonasschnelli>
If we merge at the same time... do we have really benefits?
< wumpus>
jonasschnelli: sure, you'd have to coordinate that
< provoostenator>
I hope eventually the GUI and RPC use the same interface, but that's not anytime soon...
< sipa>
as an example... btcd started out with separate repos and well-defined interfaces between wallet and node and p2p and ... and after a while they realized it's too much of a pain and moved everything into one repo
< MarcoFalke>
jonasschnelli: The majority of GUI changes hopefully don't change the interface
< wumpus>
provoostenator: that was always my preference, but it's not going to happen, face it
< sipa>
because interesting changes invariable change the interface
< jonasschnelli>
^ +1
< MarcoFalke>
I hope the interface converges over time
< jonasschnelli>
That would mean the GUI has stalled
< jonasschnelli>
(eventually)
< jonasschnelli>
(maybe not)
< wumpus>
I do think it's absurd to have everything from the consensus code to GUI in the same repo, and would like to change this
< wumpus>
but yes where to start
< jamesob>
I was going to argue that true repo separation is good because it makes us more likely to screw up the dangerous stuff (consensus, network), but I'm not even sure there's a good argument for that
< sipa>
wumpus: yes, i know
< jonasschnelli>
wumpus. Yes. I agree.
< ryanofsky>
sorry missed earlier discussion, but i think there's a lot of work can get done in gui that doesn't require changing interfaces
< jamesob>
*less likely!
< jonasschnelli>
But still,... a tiny GUI change can draw the code node down (since everything runs in the same process)
< jonasschnelli>
We still have to be careful
< jnewbery>
sipa: "if this will result in more GUI work ... more work on the GUI will entail more changes to the interface" is the good problem :)
< wumpus>
jonasschnelli: hence the process separation work
< ryanofsky>
for changes that do affect interfaces, just submit the pr in the main repo. or if it's easy just add the interface in one pr and use it in a different pr
< promag>
how would this benefit new GUIs?
< jonasschnelli>
Yes. I just wonder if it would be wiser to seperate the repositories with merging "the" process seprataion
< elichai2>
Sipa I can give an example of where it does work. In the rust compiler there's a monorepo that contains most of the complex compiler stuff and it contains submodules of interface tools (static analysis, dynamic analysis, more lints etc) and when a PR changes both repos they're linked together and after ACK on both sides they first merge the compiler side and then the tool's side.
< MarcoFalke>
promag: Right now the change does not benefit new GUIs
< wumpus>
it's only one step
< wumpus>
come on
< jonasschnelli>
Yeah. I agree that its worth a try
< jonasschnelli>
It might be simpler and more efficient that we initially think.
< MarcoFalke>
It is a step in the direction. If we can't get that done, then we shouldn't attempt any further splits imo
< jonasschnelli>
Lets try
< achow101>
I suppose that if we can easily revert it later then it's fine
< sipa>
yeah, this discussion isn't about this PR anymore but more longer-term effects
< jamesob>
heck I'm ACK. it'll be easy to revert, and if maintainers want it then so be it - they're the ones who it affects most
< ryanofsky>
yeah, it's just a minor step. i can't see how it would help anything that email filtering wouldn't help, but it seems harmless
< sipa>
i agree with this because it's so easy to revert
< jonasschnelli>
PR/email filtering is easy... that should not be the reason to split off
< wumpus>
yes, filtering is easy, that's not the point
< jonasschnelli>
review style,.. contributors should it be
< wumpus>
delegation is
< jonasschnelli>
yes
< wumpus>
I don't want to be the bottleneck for everything
< wumpus>
certainly not on the long run
< sipa>
of course
< jonasschnelli>
indeed.
< wumpus>
the bitcoi repositry is way too broad
< jb55>
I wouldn't say its that easy, there's no way to filter based on labels via email
< elichai2>
The downside of "reverting" is losing PRs/Issues history.(by having some of it in a deprecated out of date repo) Altough that's probably not a big deal
< promag>
wumpus: right, one step, I'm just wondering about the next steps
< ryanofsky>
how is this different than you just filtering out gui-tagged prs and issues?
< provoostenator>
I don't even use email for notifications :-)
< wumpus>
sigh...
< achow101>
elichai2: issues can be moved between repos now. not sure about prs
< elichai2>
achow101: you're right. Forgot about that feature
< jamesob>
jb55: agree, also curious how people are filtering gui emails out...
< MarcoFalke>
elichai2: The same pr (commit hash) can be opened against either repo
< sipa>
ryanofsky: someone has to merge things still, and i think wumpus feels responsible for that eventually
< wumpus>
yes, how are you even doing that?
< MarcoFalke>
I click on "mute conversation" manually on most gui emails
< achow101>
isn't jonasschnelli supposed to be the gui maintainer?
< jamesob>
achow101: LOL
< jonasschnelli>
I try to take care of the GUI prs...
< jonasschnelli>
though there are a lot of PR waiting for more reviewers..
< wumpus>
currently, yes
< jonasschnelli>
or are of isigificance that it doesnt attact reviewers
< jonatack>
github-cli now works quite well for filtering things by label
< jnewbery>
MarcoFalke: what's the process for merging from the GUI repo to the main repo? Do you propose that it happens immediately after a PR is merged, or do you batch it? Do all of the reviewer ACKs get lost?
< jonasschnelli>
if a GUI misses review or maintainer action,.. just point me to it.
< wumpus>
I can't be the only person why things it's, in principle, absurd for the GUI to be in the same repository as critical consensus code
< MarcoFalke>
jnewbery: The github-merge.py script does it
< jonasschnelli>
wumpus: agre
< MarcoFalke>
It is instantaneous to both repos, nothing is lost
< jamesob>
what is the anticipated burden of rerouting people filing issues/PRs in bitcoin/bitcoin to bitcoin/bitcoin-gui when appropriate?
< MarcoFalke>
jonasschnelli: I think #16432 is close to merge (off-topic)
< wumpus>
but in any case it seems I have a large disconnect with other developers in that regard
< MarcoFalke>
jamesob: A linter could do that
< jamesob>
wumpus: no I think there's broad agreement there. does *anyone* think that all else equal, gui + consensus is a good thing?
< jonasschnelli>
MarcoFalke: indeed... will take a final look
< ryanofsky>
MarcoFalke, master branch is effectively mirrored both repos?
< provoostenator>
One can make the same argument for the wallet, but that's about the only think I can think of splitting.
< MarcoFalke>
ryanofsky: Yes. monotree
< wumpus>
jamesob: I don't know, this comes up every few years and it seems besides a few people agreeing, most thing the status quo is okay
< MarcoFalke>
The gui repo only has the master branch (or tree)
< wumpus>
provoostenator: yes, one can make the same argument for the walet, but that's a discussion for another time
< provoostenator>
agreed, GUI is a good place to start
< MarcoFalke>
Yes, let's wait with the wallet until at least next year :)
< MarcoFalke>
No need to rush
< wumpus>
right, need to start somewheere and with some small step
< MarcoFalke>
Separation was suggested in 2013. At one point we need to take a small step
< wumpus>
yes...
< MarcoFalke>
(or even earlier)
< jonasschnelli>
heh
< wumpus>
2012 I think
< wumpus>
I was ther
< sipa>
you wrote the qt gui :p
< MarcoFalke>
blame wumpus
< wumpus>
biggest mistake in my life
< MarcoFalke>
xD
< sipa>
no it wasn't
< wumpus>
well ,second (but not going into details there)
< jonasschnelli>
hah
< jamesob>
lol
< jb55>
at least it wasn't an electron gui
< sipa>
imagine we'd still be stuck on a pre-release wxwindows version
< MarcoFalke>
the gui made me contribute to Core
< wumpus>
heh
< jonasschnelli>
me 2
< jonasschnelli>
the GUI is a great module to win new contributors
< aj>
jamesob: (consensus and a block-explorer gui; p2p and a p2p gui; and wallet and wallet gui make sense as individual pairs; consensus and wallet gui seem weird, but not an unreasonable consequence of us not having split consensus, p2p and wallet into separate repos)
< wumpus>
I'm sorry to not have addressed #17145 though when I could
< jonasschnelli>
without the interfaces, it was also easier to learn about the internals
< elichai2>
I know dozens of people that run full nodes only because of the gui
< jonasschnelli>
wumpus: no worries. Don't blame yourself. Things evolve. No-one would have started everything async in 2011.
< wumpus>
I'm glad to hear some people do appreciate it :)
< jb55>
elichai2: yeah I mainly use gui now due to the recent psbt/hww features
< jamesob>
aj: I see what you're getting at there - but I think the rationale is that consensus is so sensitive that you want it as isolated as is practical
< jonasschnelli>
Yeah. We should not underestimate the GUI (even if most of us devs won't use it).
< MarcoFalke>
I like the RPC console :)
< MarcoFalke>
(10 min left)
< jonasschnelli>
m2
< gwillen>
jb55: :D that's exciting to hear, and I swear I am going to go rebase #18027 today so you can use it on master :-)
< jamesob>
MarcoFalke: as fast as you can merge it buddy
< MarcoFalke>
jnewbery: I am happy to review both versions. From the perspective of the disconnected peer, they should behave identical.
< jnewbery>
the p2p-refactor-header doesn't disconnect the peer (maintains current behaviour)
< MarcoFalke>
The tests are changed, so it changes log behavior at least :)
< wumpus>
jamesob: welcome back!
< wumpus>
jnewbery: wait, isn't the disconnection the point?
< troygiorshev>
wumpus: not really. Yes from the title of the PR, but the discussion has moved to it just being a refactor of the checks into net from net_processing
< troygiorshev>
#15197 is the "other half"
< wumpus>
if a peer sends invalid data it should be disconnected imo
< wumpus>
if it's just a refactor I'm not sure it's that interesting
< wumpus>
I guess the only thing causing it to disconnect on complete garbage is the invalid messagestart bytes now
< troygiorshev>
i found in testing that most of the time garbage was being rejected on message size
< troygiorshev>
(pretty likely to have a 1 in the first 15 bits of the message size field)
< wumpus>
will it disconnect for invalid size though?
< wumpus>
m_valid_header=false doesn't seem to be a disconnect condition
< troygiorshev>
it's in readheader
< troygiorshev>
it's there so that we don't possibly read and hash >4G from the peer before disconnecting
< wumpus>
yes there's that :)
< troygiorshev>
m_valid_header effectively only pertains to the messagetype. The other two checks (size and netmagic) are done and dealt with beforehand. I think (hope) i caught all of the overlap in my branch
< achow101>
MarcoFalke: How split up do you want #18971 to be? Most of the commits are self contained and could standalone, but don't necessarily make sense by themselves. So I could make 10 PRs with 2 or so commits each, but would that really be beneficial?
< wumpus>
troygiorshev: right, invalid message types are okay to ignore
< wumpus>
explicitly shouldn't disconnect for those as it's an extension mechanism
< MarcoFalke>
achow101: Not sure. I guess reviewers can also do several afternoons reviewing 5 commits each and then sharing their intermediate review comments incrementally
< wumpus>
(though having NUL bytes in between the name, what it checks for there, is questionable, and also tends to indicate corruption or a buggy implementation)
< troygiorshev>
imo we should disconnect on those two checks (0s and invalid chars), and disconnect on checksum fail, and stay connected on an unrecognized but otherwise well-constructed message type. There was a lot of disagreement in the pr and the pr review club though. At least my branch will make it easier to do that in the future :)
< wumpus>
I guess checksum failures can *rarely* happen for users that are on really crappy networks, TCP and IP checksums will catch most corruption, it needs to be really bad for it to sometimes slip through that
< wumpus>
in the case of something like SSH that's annoying because yo ureally want to remain connected to that one host, for bitcoin P2P though ,they'll just connect to a different node
< jnewbery>
wumpus: there was lots of discussion about whether we should drop the message or disconnect in the case of a bad checksum. See here onwards if you're interested: https://bitcoincore.reviews/15206.html#l-81
< wumpus>
jnewbery: thanks!
< jnewbery>
the short answer is that jonasschnelli's PR did two things: move checksum checking from net_processing into net (definitely good), and change behaviour from drop message to disconnect peer (probably good, but at least worth discussion)
< jnewbery>
Troy's branch just does the first, so discussion of the behaviour change can be separated
< wumpus>
I understand, disconnecting a peer for one corrupted message sounds like overkill somehow, and not really robust
< wumpus>
especially as bitcoin's P2P protocol is more or less stateless and can handle lost messages
< jnewbery>
right. There was some discussion about whether an overzelous firewall could mess around with IP addresses in version or addr messages and break the checksum. Anyway, it's all in the review club notes :)
< jonasschnelli>
I start to agree with wumpus...
< jonasschnelli>
The longer i noodle about it,... the more sense it makes to close 15206
< jnewbery>
jonasschnelli: not changing behaviour is the conservative choice
< jonasschnelli>
Yeah. But the refactor alone doesn't really make much sense...
< jonasschnelli>
its nice. but not necessary anymore for future transport protocols (like BIP324)
< jnewbery>
I think it does. Moving header checking down and making net processing unaware of p2p header format is a useful thing
< jnewbery>
both for BIP324 and altnet
< jnewbery>
and just good architecture
< jonasschnelli>
Yes. I agree.
< jonasschnelli>
I strip down the PR... but it's impact and refactoring leverage is minimal.
< wumpus>
jnewbery: that's an interesting thought though: if it replaces a certain 4-byte sequence with another one, consistently, that will also interfere with some blocks and header propagation at the least
< jnewbery>
take a look at Troy's branch before you spend too much time changing yours. It's slightly fiddly to do the checking in net.cpp without causing a disconnect
< jonasschnelli>
Yes. I'll do that.
< jnewbery>
wumpus: I'm not personally convinced about the firewalls changing messages argument, but it was presented as a reason to be careful about changing behaviour
< wumpus>
jnewbery: clearly we should do scrambling + error correction at the application layer to work around network layer corruption :-)
< wumpus>
or even just scrabling, at least the bit patterns that cause packet drop would be unpredictable then
< wumpus>
in any case the only reason this is controversial at all is because bitcoin's P2P is semi-stateless so losing a packet is not necessarily fatal, in a stateful protocol the only sensible thing is to disconnect
< sipa>
many messages aren't stateless at all
< wumpus>
I know, hence the semi-
< wumpus>
otherwise the best suggestion would be to just go UDP
< sipa>
right
< wumpus>
when corruption happens in the middle of something stateful it's probably better to disconnect immediately instead of continue and wait for expiration
< wumpus>
the firewall case is statistically mostly bound to happen in addr messages of course
< bitcoin-git>
[bitcoin] ryanofsky opened pull request #19099: refactor: Move wallet methods out of chain.h and node.h (master...pr/wclient) https://github.com/bitcoin/bitcoin/pull/19099
< sipa>
and the initial version message
< sipa>
but yes... if it's in addr messages, maintaining the connection would be preferable
< wumpus>
that's an interesting one, if the checksum of the initial version message fails ... the connection will never go anywhere
< sipa>
though it could be a firewall that's only rewriting some address ranges, which only occur in randomly gossiped addressed, not the addresses of the connection partners
< wumpus>
I know it disconnects if some other message is sent as first message, but a checksum failure will probably keep open the connection
< sipa>
interestingly for clang, -O2 asm and -O3 asm seem indistinguishable
< wumpus>
weird
< wumpus>
so there, too, asm makes -O3 slower and -O2 faster
< wumpus>
but not as extreme as for gcc 10.0.1, so they end up on top of each other
< luke-jr>
could it be issues were found with some optimisers so got demoted to -O3?
< wumpus>
it wouldn't be the first time secp256k1 triggers a compiler bug, but i doubt compilers are self-aware enough to demote optimizations in that case :)
< sipa>
wumpus: hmm, i can't recall any examples
< luke-jr>
well, as long as the bug doesn't impact correctness..
< sipa>
of secp256k1 triggering a known bug
< sipa>
maybe it's some meltdown/spectre style protections that got enabled by default?
< sipa>
unsure about that
< luke-jr>
speaking of, did anyone ever figure out if we should be enabling retpolines?
< bitcoin-git>
[bitcoin] ryanofsky opened pull request #19101: refactor: remove ::vpwallets and related global variables (master...pr/novp) https://github.com/bitcoin/bitcoin/pull/19101
< wumpus>
it's probably better to leave such things to the OS/compiler
< sipa>
gcc has some flags to enable protections, but they're not enabled by default afaik
< wumpus>
likely because they make things much slower
< bitcoin-git>
[bitcoin] achow101 opened pull request #19102: wallet: Introduce and use DummyDatabase instead of dummy BerkeleyDatabase (master...true-dummydb) https://github.com/bitcoin/bitcoin/pull/19102
< luke-jr>
wumpus: the OS/compiler can't guess at what we need
< wumpus>
there's quite a few kernel-level workarounds at least that activate based on the actual CPU
< wumpus>
this mostly has to do with guarding the userspace-kernel space interface, which is a clear boundary, inside applications it's a lot less clear what you can do, and there's been such a cesspool of different vulnerabilities I'm not sure anyone does
< sipa>
applications that have JIT compiled code executed in the same process also are particularly vulnerable, but that's not the case for us either
< sipa>
or more specifically, untrusted JIT compiled code
< sipa>
(browser tab spying on another browser tab etc)
< wumpus>
yup, that's another trusted-untrusted boundary
< fanquake>
sipa: it depends on the distribution you’re using, if the flags are on by default
< fanquake>
If we were to upgrade to a newer Ubuntu for gitian builds, we’d end up with a few new protections turned on unless we start explicitly opting out.
< sipa>
fanquake: ah good to know
< sipa>
i'm on ubuntu focal, so this may have influenced my measurements
< fanquake>
Yep. They started patching new things in from unit in 19.10 onwards
< fanquake>
*Ubuntu
< fanquake>
I have a PR open with some details, but haven’t gotten to any significant benchmarking
< fanquake>
#18921
< gribble>
https://github.com/bitcoin/bitcoin/issues/18921 | build: add stack-clash and control-flow protection options to hardening flags by fanquake · Pull Request #18921 · bitcoin/bitcoin · GitHub
< fanquake>
Waiting to convince jamesob to throw it into bitcoinperf
< luke-jr>
hmm, does that suggest we ought to be enabling those things? (new Ubuntu defaults)