< warren>
meshcollider: I dunno, making bitcoin-qt work again seems controversial to me.
< meshcollider>
lol
< warren>
Is one pull request with a bunch of different unrelated backports really the best way to get people to review and test them?
< warren>
our review and testing resources are already very thin. in terms of a stable branch process, if you look at linux-kernel the stable branch maintainers seem to just merge stuff into an ongoing stable branch, which are tagged for testing and released after quality control.
< sipa>
backports basically gets no review/testing
< meshcollider>
warren: backports are generally pretty straightforward, because each commit was already reviewed in the original PR, so most backports just check its the same diff
< gleb>
Hi! I know a way to connect to a node through mininode (in regtest), but is it possible to make a cpp node connect to my mininode (direction matters)? If yes, which test has an example of that or which class I should use?
< wumpus>
yes, backports are generally very straightforward, and one PR to port over a bunch tends to work well in practice and saves on PR/merge overhead, *unless* there's a significant difference between the code base versions then it warrants creating a seprate one
< wumpus>
but I can't really remember one instance where backporting specifically went wrong
< warren>
We need to come up with a standard process for backports especially as the availability of review and testing bandwidth is very limited. In practice, adding more different things to the backports PR will make it harder to review. How many reviewers will check that it is actually the same as the original? How many will actually build and test it?
< warren>
Can tools tell us that a cherry-picked commit actually was the same as the original (applied cleanly), or it required manual merge?
< warren>
Back in pre-2010 Red Hat days I can remember one or two cases where a cleanly applied backport patch (no fuzz) led to unexpected bad behavior. It is not without risk as you know.
< sipa>
warren: yeah testing whether a cherry-pick was clean is very possible
< sipa>
i expect that most of our cherry-picks for backports are clean or very close to it
< meshcollider>
generally the changes that are backported aren't very large
< wumpus>
yes, it is possible to hek whether a cherry-pick is clean, one easy way is to repeat it and compare
< wumpus>
and yes, even cherry-picks that are clean can in some cases introduce bugs if the code around it changed subtly, no argument there, though as said in our case I've never seen it going wrong so I'm not sure it warrants extra work now
< wumpus>
we don't backport things that far usually
< wumpus>
and only *usually* minor changes, like bugfixes
< wumpus>
as meshcollider says
< meshcollider>
and of course, if we are backporting to fix a bug, we would test the binary against that bug to guarantee it did indeed fix it :)
< wumpus>
if you're in the typical redhat scenario and want to backport nowaydays fixes to 2010 era software, yes this will be lots more painful
< wumpus>
meshcollider: right-- the accompanying tests should be backprted too!
< sipa>
which generally happens
< gmaxwell>
in any case, I think sipa's point above was basically the complete answer: the backports get almost no testing, there isn't much more to say about them.
< warren>
back in 2013 I maintained a feature backport branch for stuff like watch-only and coin control. Back then I was adding to commit messages something like:
< warren>
Cherry-picked-from: <githash>
< gmaxwell>
We recommend people run current software.
< wumpus>
backporting features is not something we do, but you're free to maintain your own backport branches of course
< warren>
of course
< warren>
I meant maybe that could be a standard to help reviewers compare it more easily?
< wumpus>
warren: we have had Github-Pull: and Rebased-From: <commit> like *forever*
< warren>
ooh good
< wumpus>
it's even used by the release note generation script
< wumpus>
please, actually look at the current situation before suggesting things
< warren>
sorry yes.
< warren>
How about a way to note in the commit when something was not a clean cherry-pick? A hint toward reviewers?
< meshcollider>
if the reviewers don't notice any difference then they aren't doing a very good job reviewing
< wumpus>
there's usually a conflicts: that git generates itself
< wumpus>
which lists the conflicting files
< wumpus>
are we talking about actual problems here?
< meshcollider>
don't believe so :p
< warren>
in the case of cherrypick? Um. Maybe I hadn't used it in a while.
< wumpus>
yes, in the case of cherry-pick
< sipa>
warren: please look at actual backports
< sipa>
and how backport releases are made
< wumpus>
sipa was talking about a lack of *testing*
< wumpus>
with which I suppose he means actual human testing on platforms we dontwhich I agree is a real problem
< sipa>
yes, that's what i meant
< wumpus>
we don't CI on
< sipa>
there is little attention people give to backports
< sipa>
but on the other hand, few people use them too
< warren>
We're talking about stable point releases from a numbered tag like 0.17 right?
< sipa>
i'm talking about the creation of a 0.17.x at this point (after master is already merging for 0.18)
< warren>
0.17.0.1 was special in that it was only for Mac to install at all. But 0.17.1 with backports, as master contains merges not meant for a release so soon?
< sipa>
yes, 0.17.0.1 was speci
< sipa>
al
< sipa>
though i suspect that 0.17.1 will actually get a reasonable amount of testing
< sipa>
things like a new 0.16.x release at this point would be hard to find much interest in
< warren>
Would it make sense for the branches like 0.17 to be "living" and "not a release" until it is tagged? It could go through its own rc's with people helping it through a process. The older branch like 0.16 could also be "not a release" and very rarely have commits except if somebody bothers to backport something critical, in which case it becomes easier for people who are stuck on old versions to patch things that will probably work but it
< warren>
not being tagged signifies it didn't go through any QA process.
< gmaxwell>
it's already like that.
< warren>
(I think I'm already describing the de facto.)
< sipa>
warren: please.
< sipa>
please actually look at how things work, and identify an.actual problem
< sipa>
(it's certainly the case that some of the process around backports can be improved, but randomly shouting suggestions isn't useful for anyone)
< warren>
Err, sorry if this seems like random shouting. I'm trying to help. I see the need for more attention toward review of the commits in the stable branches and testing. Released or not those branches will be used by real people. I will help.
< wumpus>
we have the rc phase for people to find problems such as that qt issue, it's surprising no one found it and reported it there
< wumpus>
this was in 0.17.0 right? that had quite a long rc phase
< warren>
I personally didn't notice the qt issue until I upgraded into an OS that had the affected version of qt. The Gitian binary didn't have that problem right?
< gmaxwell>
warren: this issue was in 0.17.0.
< gmaxwell>
AFAIK
< warren>
I'm checking
< wumpus>
so no one tested 0.17.0rcX self-built on that particular OS
< warren>
that OS wasn't released until very recently (in the case of Fedora 29)
< wumpus>
it must have been, 0.17.0.1 changed nothing in regard to qt
< wumpus>
if so, how COULD we, or anyone, even have found that bug
< wumpus>
something breaks with yet-unreleased software
< sipa>
yet unreleased?
< warren>
Confirmed, the gitian binary had no problem because it had an older version of qt. I was testing pre-0.17 builds from master and the 0.17 branch on Fedora 28 so I didn't see the problem. I also tested the 0.17 gitian binaries.
< wumpus>
that's what warren suggests, taht F29 not out at that time
< gmaxwell>
obviously we should "Use Weapon" </arrival>
< sipa>
oh, F29 issue, not macos issue fixed in 0.17.0.1
< wumpus>
hehe
< sipa>
warren: in that case i don't think anyone is to blame; we can only test on existing platforms
< wumpus>
sipa: yes, sorry, was mixing them up though the mac issue would have been good to catch in rc too!
< sipa>
and we can also issue a 0.17.0.2 to fix the f29 issue if serious enough
< wumpus>
sure
< sipa>
i don't think that's a test failure; just something unpredictable breaking things from under us
< warren>
I wouldn't bother to do another .0.x release, just put that tiny commit in the 0.17 branch for now. People who build from source can tolerate following a branch, while most people will probably use the gitian binary.
< gmaxwell>
warren: thats normally what happens for bugfixes which are not totally unimportant.
< warren>
How extensive will 0.17.1 be?
< wumpus>
warren: yes that should happen, do you have the PR# I'll do a backport PR
< bitcoin-git>
bitcoin/0.17 0242b5a Tim Ruffing: qt: Revert "Force TLS1.0+ for SSL connections"...
< bitcoin-git>
bitcoin/0.17 b0e88b8 Wladimir J. van der Laan: Merge #14666: qt: Revert "Force TLS1.0+ for SSL connections" (0.17)...
< meshcollider>
that was quick :D
< wumpus>
very happy to see people caring about actually fixing bugs and not *insert a space at row 4 column 23 of doc/bla.md*
< meshcollider>
heh yep
< wumpus>
luke-jr: need rebase for #14532, also other people: this PR addresses an important issue and still needs review
< gribble>
https://github.com/bitcoin/bitcoin/issues/14532 | Never bind INADDR_ANY by default, and warn when doing so explicitly by luke-jr · Pull Request #14532 · bitcoin/bitcoin · GitHub
< bitcoin-git>
bitcoin/0.17 1e49fe4 Wladimir J. van der Laan: doc: Clean out release notes after 0.17.0.1...
< warren>
luke-jr: "(A warning is given if rpcallowip is specified without rpcbind, since it doesn't really make sense to do.)" ... might it be better to change it to refuse to start bitcoind after this change? People depending on the previous wrong behavior will more quickly notice bitcoind failing to start this way.
< meshcollider>
warren: it doesn't default to old behaviour though, the behaviour is still safe so they will notice if they were relying on it anyway
< meshcollider>
i.e., it still won't bind to INADDR_ANY
< warren>
meshcollider: the wrong behavior was rpcallowip without rpcbind, luke-jr argues
< wumpus>
I think things are ready now to start the process for a 0.17.0.2, though, I'm not convinced we should, because it only affects self-built executables on one fairly rare platform and doesn't affect the binaries nor anyone using the binaries, might be too much confusing as well as give a bad impression to do a lot of 0.x releases
< warren>
Not sure if this was ever written anywhere, but I personally always understood the RPC port was never meant to be exposed to the Internet.
< warren>
wumpus: I would call it rather an "currently rare but over time increasingly less so", but even then yes the static linked qt in the gitian binaries are not affected and we're better off getting 0.17.1 out the door.
< wumpus>
you're right, it's never supposed to-the problem is that a lot of people were apparently accidentally exposing it to the internet, might be partially because they set rpcallowip to a local-network address then suddenly expose it to the whole world, not doing automatic binding would prevent that
< wumpus>
I *wish* bitcoind had c-lightning's RPC, simply a UNIX socket that takes JSON-RPC commands, no HTTP, no binding
< meshcollider>
yeah IMO this doesn't warrant a 0.17.0.2 because it doesn't affect that actual binaries
< wumpus>
meshcollider: right
< gwillen>
wumpus: the HTTP layer has been slightly maddening in talking to bitcoind RPC from Rust
< wumpus>
gwillen: I know :(
< warren>
Does c-lightning RPC work on Windows? (Why bother I know.)
< gwillen>
there are currently zero reasonable HTTP clients available
< gwillen>
(for values of reasonable that include minimizing dependency bloat primarily)
< wumpus>
I don't think it supports windows, another good thing about it :-)
< gwillen>
well, you can take JSON-RPC commands over TCP from localhost as easily as over UNIX sockets, no HTTP required
< luke-jr>
warren: It's not *entirely* nonsensical, though.
< luke-jr>
gwillen: dependencies are good, not bloat
< wumpus>
not if it pulls in an apache-level huge web server just to handle a silly RPC client
< warren>
luke-jr: I too am in favor of infrastructure applications with webs of dependencies blindly grabbing from other developers.
< luke-jr>
wumpus: sure, but the topic was a HTTP client :P
< luke-jr>
eg, libcurl
< wumpus>
luke-jr: I'm just saying what the practical situation for Rust is
< warren>
libcurl is rather large and has a history of security issues too.
< luke-jr>
surely Rust can have bindings to reasonable C libraries?
< luke-jr>
warren: everything has security issues at some point
< wumpus>
using C at the network layer partially defeats the point of using RUst in the first place :-)
< luke-jr>
using a common dependency at least increases the probability it gets found and fixed quickly
< wumpus>
as warren says
< wumpus>
libcurl is not exactly bugless either
< warren>
You'd think a rust thing wanting something simple as socket communication you wouldn't want to pull in a massive library, rather you'd want to implement it in a safer language like rust, with as minimal as possible in code.
< wumpus>
though, hardening against network exploits is much more important for something like the P2P network code than *what is supposed to be* a local control socket, but that has a much bigger scope, but some people are working on writing a Rust P2P layer too
< bitcoin-git>
bitcoin/master bbbbb3f MarcoFalke: qa: Add test to ensure node can generate all help texts at runtime
< bitcoin-git>
bitcoin/master cdddd17 Wladimir J. van der Laan: Merge #14658: qa: Add test to ensure node can generate all rpc help texts at runtime...
< bitcoin-git>
[bitcoin] laanwj closed pull request #14658: qa: Add test to ensure node can generate all rpc help texts at runtime (master...Mf1811-qaRpcHelpMan) https://github.com/bitcoin/bitcoin/pull/14658
< promag>
bugfix #14552 lgtm
< gribble>
https://github.com/bitcoin/bitcoin/issues/14552 | wallet: detecting duplicate wallet by comparing the db filename. by ken2812221 · Pull Request #14552 · bitcoin/bitcoin · GitHub
< setpill>
trying to build bitcoin core in a bionic host with the gitian-build.py script, getting error `lxc-init: gitian: cmd/lxc_init.c: remove_self: 212 Invalid argument - Failed to unmount "/usr/sbin/init.lxc.static"`
< setpill>
is there a known workaround for this?
< wumpus>
"if you forget to install libcap-dev before building lxc, lxc-execute is going to fail because an essential program init.lxc.static is not built"
< wumpus>
I had that problem once, maybe it's this, maybe it's a different one
< setpill>
I'm not building lxc, just installing from repositories
< wumpus>
in that case, they might have made that mistake, in any case no clue
< wumpus>
sooo many moving parts that can break, who's to say...
< bitcoin-git>
bitcoin/master 4fb789e Jim Posen: Extract CSipHasher to it's own file in crypto/ directory....
< bitcoin-git>
bitcoin/master fef5adc Jim Posen: blockfilter: Use unordered_set instead of set in blockfilter.
< bitcoin-git>
bitcoin/master 880bc72 Wladimir J. van der Laan: Merge #14074: Use std::unordered_set instead of set in blockfilter interface...
< bitcoin-git>
[bitcoin] laanwj closed pull request #14074: Use std::unordered_set instead of set in blockfilter interface (master...blockfilter-unordered-set) https://github.com/bitcoin/bitcoin/pull/14074
< bitcoin-git>
bitcoin/master 5a05aa2 Martin Erlandsson: Add metavar to match var name in help text + Change wording for better readability
< bitcoin-git>
bitcoin/master 6af27b8 MarcoFalke: Merge #14619: tests: Fix value display name in test_runner help text...
< jamesob>
my just-merged p2p_invalid_messages test is failing pretty consistently on travis; looking into it
< bitcoin-git>
[bitcoin] MarcoFalke closed pull request #14619: tests: Fix value display name in test_runner help text (master...test-runner-fix) https://github.com/bitcoin/bitcoin/pull/14619
< bitcoin-git>
[bitcoin] jamesob opened pull request #14672: tests: Send fewer spam messages in p2p_invalid_messages (master...2018-11-msgs-test-fix) https://github.com/bitcoin/bitcoin/pull/14672
< warren>
dongcarl: he mentioned that earlier, it actually works now?
< warren>
dongcarl: oh good it looks like devrandom has already been trying it
< hebasto>
latest master build behaves like -disablewallet=1 always; what was changed? (linux, testnet)
< esotericnonsense>
i've been spamming this around a bit, but again for anyone that cares, my node/mempool monitoring daemon is finally open source (it wasn't really 'deployment ready' previously, took a while to wrangle it in to shape)
< esotericnonsense>
i think that the defaults are sane; on initial setup it grabs some bytes from /dev/urandom for rpc passwords and so on (not exposed outside localhost anyway), and then I've limited remote RPC access for the proxy to a subset that I think are reasonable (there's a whitelist)
< esotericnonsense>
if it had an auth gate (and probably some sort of self-sanity-check to make sure it's on TLS/SSL) more stuff like wallet functionality, full debug console etc could be exposed
< dongcarl>
warren: It doesn't like Arch Linux's OpenSSL version... Too new
< phantomcircuit>
sdaftuar, any idea why it's not working on os x?
< bitcoin-git>
[bitcoin] practicalswift opened pull request #14673: travis: Fail the UBSan Travis build in case of newly introduced UBSan errors (master...ubsan-tweak) https://github.com/bitcoin/bitcoin/pull/14673
< bitcoin-git>
bitcoin/master 5c292da practicalswift: Add UBSan suppressions needed to pass test suite
< bitcoin-git>
bitcoin/master 4773fa8 practicalswift: Add llvm-symbolizer directory to PATH. Needed to get symbolized stack traces from the sanitizers.
< bitcoin-git>
[bitcoin] MarcoFalke closed pull request #14673: travis: Fail the UBSan Travis build in case of newly introduced UBSan errors (master...ubsan-tweak) https://github.com/bitcoin/bitcoin/pull/14673