< theStack>
has anyone ever used the functional test framework to interact with an already running full node? for some (performance) tests it would make sense to use the mainnet i think
< fjahr>
theStack: have you seen the TestShell class from James Chiang? It is used in the jupyter notebooks for the optech Taproot course, that might be helpful for you.
< theStack>
fjahr: thanks for the hint, will take a look at it!
< fjahr>
I am not sure about the already running part though. Should probably work if you just change the pid? But I have not tried it.
< bitcoin-git>
[bitcoin] MarcoFalke opened pull request #18974: test: Check that invalid witness destinations can not be imported (master...2005-testInvalidWitnessDestination) https://github.com/bitcoin/bitcoin/pull/18974
< fanquake>
promag: can you check the coin control backports in #18973
< theStack>
fjahr: the interesting part for me would be that it doesn't start a new chain from scratch; if i need 100000s of blocks for performance tests starting a new regtest chain is not an option
< theStack>
creating a TestNode() instance with right parameters could maybe work
< jnewbery>
were you planning to do some perf testing for 18960?
< fanquake>
MarcoFalke: is there anything obvious to backport into the 0.20 branch to fix the test failures in 18973. Looks like it's mostly in the valgrind builds
< theStack>
jnewbery: sounds like exactly what i need, will take a look. thanks!
< jnewbery>
fanquake: #18899 removes the valgrind build from travis because it was timing out jobs
< theStack>
jnewbery: yes indeed i was planning to repeatedly fetch the bip 157 checkpoint headers, comparing how long it takes in master and 18960 branches
< theStack>
(maybe also compare it with the original proposed solutions by jimpo)
< fanquake>
jnewbery: guess that might be worth backporting as well then
< jnewbery>
theStack: great. Thanks! The first time you send a getcfcheckpt, it's reading the values from disk, so it should be slower than the second and subsequent requests
< fanquake>
jnewbery: if you've got test backport suggestions I'm all ears. I see some process_message related in the fuzzer, which I assume we've fixed.
< fanquake>
I'm going to untag #18287 for 0.20.0. Only myself and sipsorcery have tested it, and neither of us could verify that the change is fixing the actual problem. The patch does fix libevents getaddrinfo detection, but Core still doesn't behave as expected; so I think this needs further testing, and shouldn't be bundled in last minute.
< theStack>
jnewbery: do you think that is also true for the version without the cache implementation (i.e. master branch)? i don't know if BDB does caching internally (or maybe even the OS?), but there must be something as some people have expressed their doubt if the cache implementation is really improving anything
< theStack>
but anyways, i will hopefully find out soon through the benchmarks :)
< vasild>
ResolveSubNet("::/0").Match(ResolveIP("1.2.3.4")) -- is true
< vasild>
ResolveSubNet("::AAAA:0:0/80").Match(ResolveIP("1.2.3.4")) -- is true
< vasild>
but
< vasild>
ResolveSubNet("::FFFF:0:0/96").Match(ResolveIP("1.2.3.4")) -- is false
< vasild>
is this a bug?
< vasild>
I think it is strange to match IPv4 addresses as belonging to some IPv6 network, so IMO all should be false. But if I accept that we should, for some reason, match IPv4 addresses as belonging to some IPv6 networks, then also the last one should be true.
< vasild>
It is not true because "::FFFF:0:0" is interpreted as an IPv4 address (0.0.0.0) and the netmask /96 bricks it. It is the same as "0.0.0.0/96".
< wumpus>
it's somewhat strange, but it's a by-effect of IPv4 addresses have always been encoded as a kind of IPv6 address in bitcoin
< wumpus>
not sure this should affect anything in practice
< vasild>
What about stopping to accept IPv4 as IPv6 addresses? That is - all of the above return false?
< vasild>
Sorry, "stopping to accept IPv4 as belonging to some IPv6 networks"
< vasild>
I am modifying this code wrt ADDRv2 and the new code would return false for all of the above 3 tests. So I wonder if this change in behavior would be ok. In order to mimic the current (strange) behavor a few twists and tweaks would have to be made to the new code, making it less straight forward and a bit more difficult to review.
< vasild>
I will go for "no change in behavior" (+some twists in the patch). If it turns out that we don't need to accept IPv4 as part of IPv6 networks (e.g. `::/0`), then the patch can be simplified.
< wumpus>
I think that's fine, I don't think it'll affect anything in practice
< wumpus>
there's no reason to do mixed IPv6 IPv4 subnet matching
< wumpus>
so having it always return false if someone tries seems ok to me
< vasild>
ah, excellent!
< Pavlenex>
Hello. I've asked before but got no reply. Serbian translation for 0.20 is ready, it was translated from scratch basically as the language never got much translators. However the language does not have a coordinator or reviewer so it can't be pushed as complete. Who is in charge of Transifex that can provide me with an access so we can proceed? Thanks.
< wumpus>
Pavlenex: I should be able to set someone as coordinator
< Pavlenex>
wumpus: Hey! https://www.transifex.com/user/profile/nedved/ is my transifex username. However if you want to set somebody else from that team instead, it's fine as long as we can proceed with a review.
< Pavlenex>
If I can suggest alternative member that would be casperBGD, he continued on my initial efforts and completed the translations. Thanks in advance.
< bitcoin-git>
[bitcoin] laanwj opened pull request #18975: test: Remove const to work around compiler error on xenial (master...2020_05_xenial_compile_issue) https://github.com/bitcoin/bitcoin/pull/18975
< bitcoin-git>
[bitcoin] MarcoFalke opened pull request #18976: test: Add {} to force default initialization (master...2005-testDefaultInit) https://github.com/bitcoin/bitcoin/pull/18976
< bitcoin-git>
[bitcoin] MarcoFalke closed pull request #18976: test: Add {} to force default initialization (master...2005-testDefaultInit) https://github.com/bitcoin/bitcoin/pull/18976
< theStack>
jnewbery: i posted the results of my getcfcheckpt cache (#18960) research in the PR thread, with a link to the script; there doesn't seem to be any difference, any request up to block 630000 takes 50ms on my machine
< bitcoin-git>
bitcoin/master 7467366 John Newbery: [net processing] Only send a getheaders for one block in an INV
< bitcoin-git>
bitcoin/master 553bb3f Wladimir J. van der Laan: Merge #18962: net processing: Only send a getheaders for one block in an I...
< bitcoin-git>
[bitcoin] laanwj merged pull request #18962: net processing: Only send a getheaders for one block in an INV (master...2020-05-limit-block-inv) https://github.com/bitcoin/bitcoin/pull/18962
< wumpus>
#startmeeting
< lightningbot>
Meeting started Thu May 14 19:00:31 2020 UTC. The chair is wumpus. Information about MeetBot at http://wiki.debian.org/MeetBot.
< sipa>
and one of them would rely on boost multi_index's ranked indexes
< sipa>
which were added in boost 1.59
< sipa>
so i was wondering if that's a possibility for 0.21
< sipa>
or if i should consider other options (which i may do anyway)
< wumpus>
that seems a fair enough reason
< * luke-jr>
grumbles about RHEL/CentOS packages being hard to search
< wumpus>
I think the most important thing to check is the versions in currently supported linux distros for building
< wumpus>
"Version 1.59.0 August 13th, 2015 15:23 GMT" that sounds old enough, but knowing what old crap some distros ship with, it doesn't say everything
< sipa>
well if we're going to be building with c++17, it seems we'd be relying on compilers newer than that anyway
< jeremyrubin>
sipa: it's the log n erase that's more worrisome
< wumpus>
luke-jr: sure, but we're not going to hold up the rc on it
< luke-jr>
wumpus: it's a security violation for users who build from source
< luke-jr>
to answer how relevant it is
< wumpus>
how so?
< luke-jr>
and can result in leaking user private info to the network
< luke-jr>
wumpus: it's looking at every parent directory to the source code for a git repo
< wumpus>
I mean, it's been absurd how long it's taking to roll this release
< luke-jr>
and taking whatever HEAD hash it finds into the version number
< luke-jr>
pulling a hash out of an unrelated git repo the user might have, and publishing it, isn't very nice
< wumpus>
I agree
< sipa>
that sounds like something we should fix
< luke-jr>
(Gentoo will kill the build instead)
< wumpus>
but honestly we should cut a release at some point, there's aalways some edge case
< luke-jr>
this is a regression, too
< wumpus>
also #18902 rewrites a large part of the gitian descriptor and changes 6 files, can't this be fixed with some small patch?
< gribble>
https://github.com/bitcoin/bitcoin/issues/18902 | Bugfix: Only use git for build info if the repository is actually the right one by luke-jr · Pull Request #18902 · bitcoin/bitcoin · GitHub
< luke-jr>
probably
< luke-jr>
I was trying to avoid conflicts with the autogen fix, but I guess we're missing that?
< wumpus>
in any case: I agree it should be fixed, but I don't think it's terribly urgent
< hebasto>
as a workaround a user could use BITCOIN_GENBUILD_NO_GIT variable
< luke-jr>
maybe a release notes mention of the known issues
< wumpus>
yes
< wumpus>
I think that's fine
< luke-jr>
current draft is in wiki still? or git?
< luke-jr>
k, I'll see about writing something up for that
< wumpus>
thanks!
< luke-jr>
should I reduce the footprint of #18902 and/or #18909 also?
< gribble>
https://github.com/bitcoin/bitcoin/issues/18902 | Bugfix: Only use git for build info if the repository is actually the right one by luke-jr · Pull Request #18902 · bitcoin/bitcoin · GitHub
< luke-jr>
would be nice to just get 18902 merged to master as-is to fix all the issues at once :x but maybe reducing 18909 makes more sense
< wumpus>
fwiw the git_check_in_repo() check looks straightforward enough
< wumpus>
why isn't that enough?
< wumpus>
(or alternatively, check if a .git is in the top level of the source directory)
< wumpus>
besides that change in genbuild.sh, are any other changes needed to fix this?
< hebasto>
it is all about "version hack"
< hebasto>
in gitian builds
< luke-jr>
wumpus: we'd need to restore the "version hack"
< wumpus>
luke-jr talks about a security vulnerability importing data from a git repository above the bitcoin one, this can be avoided by checking in genbuild that we're really in a git repo right?
< luke-jr>
the genbuild changes in 18902 are a proper fix for that
< wumpus>
(I mean, in the right git repo)
< wumpus>
yes my question is why isn't that the only change needed?
< wumpus>
hebasto: so we can fix this by reverting a change too?
< hebasto>
#18349 probably has simpler approach
< luke-jr>
those are the only commits specific to 18902 - the other 3 are inherited from 18818
< wumpus>
it really looks like the only build configuration used and supported in practice is building from git, everything else is fragile at most
< wumpus>
FWIW for releases *and RCs all the version information in in configure.ac
< wumpus>
I'm not sure what it gets from git at all
< sipa>
random brainstorming (and highly unsure it's worth spending time on): what if we create a file that contains a list of (sha512 tree hash, version string) pairs, and at build time we compare the tree with that file, and use its name if found, tree hash otherwise
< sipa>
that would disentangle version naming entirely from git
< wumpus>
that's pretty much what git tags are
< wumpus>
I mean all the version information is in configure.ac. My point is I don't see why any external information is needed.
< wumpus>
neither from git tags nor any external file
< sipa>
fair
< wumpus>
this used to be differnt, to be clear, at some point the build process derives the rcX from the git tag
< wumpus>
but we've already made everything explicit now
< sipa>
oh, ok
< wumpus>
derived*
< sipa>
i was not aware of that
< wumpus>
no worries, I don't have a full grasp in the version logic actually used either :)
< luke-jr>
wumpus: but then we can't build non-releases with correct version info?
< wumpus>
luke-jr: what is 'correct version info' in that case anyway?
< luke-jr>
[19:50:46] <wumpus> it really looks like the only build configuration used and supported in practice is building from git, everything else is fragile at most <-- I suppose we could just not publish the tarball
< luke-jr>
wumpus: a commit hash abbreviation seems fine
< wumpus>
well the tarballs are fine, they are for releases and thus contain correct version info
< luke-jr>
something like git-describe
< wumpus>
they don't need *any* reference to git
< luke-jr>
wumpus: currently, they don't..
< wumpus>
luke-jr: they contain configure.ac, which has all the version info
< wumpus>
including rc level
< wumpus>
I know this because I have to set this every time before a rc
< jeremyrubin>
(Xenial is 16.04 LTS, which goes out of mantenance and to security only in 2021
< jeremyrubin>
Personally I think it's probably OK given that it's 4 LTS's old, but it is still in maintenance so conceivable someone is relying on it still...
< jeremyrubin>
err 3 LTS's old, not 4 (or 2 depending on how you count the current one)
< MarcoFalke>
copy the hpp to our repo? I wanted to remove boost for 0.22.0
< sipa>
remove boost? how are you going to do that?
< jeremyrubin>
MarcoFalke: IDK if it's feasible givent that multiindex uses like all of boost internally.
< MarcoFalke>
Switch to c++17 ;)
< MarcoFalke>
If we need a single boost file we could subtree that single file?
< sipa>
we're not going to start maintaining our own copy of boost to get around depending on boost
< sipa>
it's a zillion .hpp files
< sipa>
across various boost packages
< jeremyrubin>
MarcoFalke: I don't think that's feasible, you can try bcp'ing multiindex out and it's big
< jeremyrubin>
Oh 78? I thought surely it was more than 80, so let's do it!
< sipa>
which depends on boost mpl, boost move, a bunch of shared boost utility stuff, ...
< MarcoFalke>
fanquake: Looks like you can do your bump ^
< jeremyrubin>
MarcoFalke: OTOH rewriting multiindex for our specific use cases seems doable.
< sipa>
boost mpl is 1045 files...
< fanquake>
👀
< MarcoFalke>
jeremyrubin: Can you write one in less than 100 LOC (clang-formatted)?
< sipa>
lol
< sipa>
i do not want to write and maintain a copy of multi_index
< jeremyrubin>
You can't even define a concrete type for the one we have in less than 100 LOC
< sipa>
wahaha
< jeremyrubin>
sipa: I think it wouldn't be writing a custom one, but writing a non template concrete impl for the one that we currently use.
< sipa>
jeremyrubin: i spent 2 weeks trying to design a custom data structure for the thing i want :)
< sipa>
and gave up
< jeremyrubin>
Yeah. I'm just prodding MarcoFalke if he wants to ever get rid of boost
< sipa>
i mean, it obviously could be done
< sipa>
but it'd be far harder to review, and a properly abstracted version would like tend to have some minimal multi_index like reimplementation anyway
< luke-jr>
sipa: jeremyrubin: since when do we support anything more than the latest LTS?
< sipa>
MarcoFalke: i think as an intermediate goal maybe it's worth trying to only have header-only boost dependencies
< luke-jr>
MarcoFalke: copying the hpp is strictly worse than depending on boost
< luke-jr>
removing boost doesn't seem like a goal
< luke-jr>
just using C++11 (or C++17 eventually) instead
< sipa>
i think replacing boost with standard replacements when they're appropriate is great
< sipa>
removing boost at all costs seems counter-productive
< luke-jr>
exactly
< luke-jr>
and we want to add boost::process soon for several things
< luke-jr>
which AFAIK there is no C++ std alternative to
< MarcoFalke>
And in reply to "removing boost should not be a goal". I think I disagree. Most of boost seems unmaintained at this point. For example the bug that crashed Bitcoin Core was reported to upstream with steps to reproduce and even a fix. I am still waiting for a reply...