< bitcoin-git>
[bitcoin] fanquake closed pull request #22336: [TESTBED][NO-MERGE][POC] Use std::filesystem. Remove Boost Filesystem & System (master...feature/use_std_filesystem_testbed) https://github.com/bitcoin/bitcoin/pull/22336
< bitcoin-git>
[bitcoin] dongcarl opened pull request #22465: guix: Pin kernel-header version, time-machine to upstream 1.3.0 commit (master...2021-07-guix-kernel-old) https://github.com/bitcoin/bitcoin/pull/22465
< fanquake>
robertspigler: there are a number of benefits that come with using Guix as a release build environment (or just as a build system in general).
< fanquake>
Using gitian, you have to pick a Linux distro to build on. This defines your toolchains, available packages, the glibc your building against etc.
< fanquake>
This also means you're subject to choices of the upstream package maintainers. They control how/when package versions are updated, what patches are applied to them, how they are compiled etc. This has downsides.
< fanquake>
As an example, our security-check tests rely on a number of -no-* options being present in mingw-w64 ld. These options were patched into the ld in the Ubuntu Bionic binutils-mingw-w64 package, however, the binutils maintainer chose to stop doing that in Ubuntu Focal, which meant our security-check tests would not have been able to run (they have since decided to re-patch them in Hirsute). See #18629 for more details.
< fanquake>
In Guix, because we're in full control of our toolchains, we don't have to worry about issues like this, and can just directly apply the patches we want to use, and this is exactly what we've done in #22381, where we started patching the -no-* linker options into our mingw-w64 toolchain. Compiler / linker options or defaults that we may rely on, can no-longer be randomly changing out from underneath us.
<@gribble>
https://github.com/bitcoin/bitcoin/issues/22381 | guix: Test security-check sanity before performing them (with macOS) by fanquake · Pull Request #22381 · bitcoin/bitcoin · GitHub
< fanquake>
To that, some might say "well just don't change the Ubuntu base image, and things won't change". To that, I'd say, I don't want the project to be in a position where we are "stuck", and can't update our base image to use new tools (i.e compilers), because by doing so we'll just break something else, like our security tests.
< fanquake>
It would seem smarter to just remove the potential for those kinds of problem entirely, by constructing and using exactly the release build environment we want.
< fanquake>
"Well why not just patch / change things in gitian" - I wont say much on this, except for, the gitian environment is not at all setup to do the same kind of patching that we can fairly trivially achieve in Guix. Trying to patch and compile, for example, the mingw-w64 toolchain in a gitian descriptor, would also just turn into a terrible mess.
< fanquake>
The same can be said about trying to gitian build anything using a glibc different than what is already available on that version of Ubuntu.
< fanquake>
This has backward-compatibility implications, as the version of glibc you're building against, essentially determines your runtime glibc compatiblity. However this can be extended by using the sorts of "workarounds" that we currently have in our glibc-back-compat code.
< fanquake>
Now, similar to the situation where you might want to use a newer Ubuntu release (for any number of reasons), doing so will mean that you have to build against a newer glibc (you just get whatever version comes with that release of Ubuntu).
< fanquake>
This means that as you use newer versions of glibc, the number of "workarounds" you need to maintain backwards compatibility pile up, get continually more complicated, and even start to leak out of Bitcoin Core code, and into our dependency system. See all the PRs linked in this comment: https://github.com/bitcoin/bitcoin/pull/22418#issuecomment-876379846.
< fanquake>
Changes like that leaking into depends are bad, because now normals builders, using depends, are now being subject to the side-effects of patches that are only really necessary in our release build environment. Maybe you'd argue that in that case we should only apply the depends patches when building releases, however then you've got a situation where release builds are even more divergent from "normal"/developer builds, and this means
< fanquake>
either maintaining an even more complex CI / testing routines, or they just end up less tested (guess which one is more likely to happen).
< fanquake>
If that all just sounds like a complicated mess, it's basically because it is. However the solution, using Guix, is actually pretty straight forward.
< fanquake>
When we are in complete control of our release environment, we can pick exactly the version of glibc we want to use (even at a per-HOST level), something that we definitely couldn't achieve, in any sort of straight forward fashion, if at all, using gitian.
< fanquake>
This is something we've done recently, in #22365. We are now building using glibc 2.27 for the RISC-V HOST, and using glibc 2.24 for all others, while maintaining runtime compatibility with glibc 2.17. You'll note this this is achieved without needing to use any of the work arounds from our glibc-back-compat code (#22405), or any of the other PRs / changes mentioned in the comment above.
<@gribble>
https://github.com/bitcoin/bitcoin/issues/22365 | guix: Avoid relying on newer symbols by rebasing our cross toolchains on older glibcs by dongcarl · Pull Request #22365 · bitcoin/bitcoin · GitHub
< fanquake>
These are just two very practical benefits that using Guix provides (there are more), which ultimately all boil down to us being in much greater control of our release build environment. Something I am very happy about, and I think makes a lot of sense for a project like Bitcoin Core.
< fanquake>
The fact that there are also many people in the Guix space actively working on bootstrapability is just another big bonus, and even if that doesn't work exactly like some may want it too right now, it is rapidly being improved, month on month.
< fanquake>
Guix is also a much more likely pathway to fully bootstrapable Bitcoin Core builds that what gitian could ever provide.
< dongcarl>
👏
< robertspigler>
fanquake: That's great, thanks for the detailed explanation! I didn't know there were benefits beyond bootstrappability. So that makes me even more confused re: why run guix in gitian?
< dongcarl>
robertspigler: I don't think we're going to run Guix inside Gitian for the releases, but personally I think people are free to do whatever they think make sense if they're committed to maintaining it
< robertspigler>
Sure, I agree with that
< robertspigler>
I was wondering if I was missing something, but it doesn't seem that I am
< luke-jr>
robertspigler: you don't lose those Guix benefits by running it inside gitian; gitian just provides the isolation needed to avoid regressing on security
< vasild>
MarcoFalke: do you think that the fuzzer inputs from #22450 should be collected and added to bitcoin-core/qa-assets ?
< laanwj>
as not much is left for rc1, we could branch off 22.0 so that the master branch is free for merging for 23.0
< jnewbery>
vasild: what do you think about just removing any non-port0 I2P addresses from addrman in RemoveInvalid()? Seems a lot safer than a bunch of new code in ResetI2PPorts()
< vasild>
all of them are with port 8333, so that would be the same as just removing all I2P addresses from addrman (done just once)
< vasild>
hmm, and because some peers will keep advertising themselves with port 8333, we will keep removing them
< vasild>
jnewbery: actually removing from addrman was tricky, IIRC, there is no Delete() method
< vasild>
if we are to remove them, we may as well re-add with port 0, like you suggested
< vasild>
yes, and it does the same as ResetI2PPorts() - fiddle with addrman internals
< jonatack>
One thought. This change was fairly well-tested over a period of time; 3 people ran it for several days each (in my case, with a number of added asserts and with -DDEBUG_ADDRMAN). A re-do might see less testing.
< vasild>
jnewbery: https://github.com/bitcoin/bitcoin/issues/22450#issuecomment-880602891 is a 3rd issue, I have not confirmed it yet, but read the comment: yes, it looks like it can cause a corruption, but again that is a 3rd distinct problem. I suspect it may be resolved by just removing "addr_info.GetPort() == I2P_SAM31_PORT"
< bitcoin-git>
[bitcoin] vasild opened pull request #22468: addrman: don't overwrite addr_info when resetting I2P ports (master...reset_i2p_ports_no_overwrite_pos) https://github.com/bitcoin/bitcoin/pull/22468
< MarcoFalke>
> [09:43] <vasild> MarcoFalke: do you think that the fuzzer inputs from #22450 should be collected and added to bitcoin-core/qa-assets ?
< jnewbery>
vasild: I think the entire approach in ResetI2PPorts() is scary. Even if you fix these corruption issues, how do you know there aren't more?
< vasild>
jnewbery: I can't be sure there aren't more bugs. But that is also true for any software.
< vasild>
Do you think RemoveInvalid() is ok (not scary)? It is very similar to ResetI2PPorts() in the way they iterate over the buckets and do something to relevant entries
< jnewbery>
RemoveInvalid() seems better to me in that it's doing less to the internal data structures. I'd like it even more if Delete() was extended to support deleting tried entries, so there's less code duplication. It also got more review before being merged, which gives me some more confidence.
< vasild>
"I'd like it even more if Delete() was extended..." me too! It need to be extended not only to delete tried entries but also new ones that have reference>0
< vasild>
the current one should be called something like DeleteNewWithRef0OrIWillAssert()
< jonatack>
at any rate I continue to run and test the changes with Check_() e.g. DEBUG_ADDRMAN and some sanity check asserts added and will look at #22468.
< vasild>
sipa_: if we revert that part (which changes ports in addrman from 8333 to 0) then we would break the existent i2p network in a subtle way: all i2p addresses in addrman are with port 8333 and we would refuse to connect to any of them. Then those peers run the new code and start advertising themselves with port=0 however when that gossip reaches a node that already has the address but with port
< vasild>
8333, that node will ignore the incoming entry with port=0 and keep the old one with port 8333 (to which it refuses to connect)
< vasild>
the only solution (I see) would be for everybody to re-generate a new i2p address
< sipa_>
vasild: but i2p support is only on master, right?
< vasild>
It _should_ be possible to properly change the ports from 8333 to 0 in addrman.
< vasild>
sipa_: yes, it is not in any previous releases, but there are already a bunch of nodes and in seeds.txt, so the i2p bitcoin network exists
< sipa_>
sure, but it's just people running master
< vasild>
correct
< sipa_>
we can include a snippet in the release notes
< sipa>
i agree it ought to be possible to hotfix it directly in addrman, but i'm concerned about the fact that inconsistencies have been introduced already
< vasild>
sipa: you mean that there are already bugs in that code or that some nodes already run with inconsistent addrmans?
< sipa>
i'm wary about touching addrman more, as it has already been shown it's easy to corrupt
< sipa>
though i'll have a deeper look at the issue and the proposed fixes too
< vasild>
there are 3 issues reported in #22450 (sorry, that's messy): 1. the description/OP, fixed in #22455 (not relevant to I2P)
< vasild>
A general note about feeding fuzzed input to addrman unserialize: it could result in some unexpected state, like negative nNew coming from disk or duplicate entries which are not allowed during normal operation. Surely it should not crash when unserialized from a random input, as a disk corruption could lead to that too.
< vasild>
=> I think there is a good chance fuzzing addrman unserialize will find more bugs
< sipa>
definitely
< bitcoin-git>
[bitcoin] vasild opened pull request #22471: addrman: reset I2P ports in all "new" buckets (master...reset_all_new_i2p_ports) https://github.com/bitcoin/bitcoin/pull/22471
< jnewbery_>
vasild: I'll repeat that I think it's much safer to just delete these entries on startup using the existing RemoveInvalid() code. There are at least two addrman corruption bugs introduced by the ResetI2PPorts() code in #22112. What level of confidence do you have that there aren't more?
< vasild>
jnewbery: now that we have a fix for everything, I will also look into that suggestion as an alternative fix for #22467 and #22470. But I don't have high hopes as I tried that and it was not simpler. So, "just delete" will expand to similar number of lines as now in ResetI2PPorts().
< vasild>
notice also that RemoveInvalid() was also added recently, so it is not "sure" that it is safe either, e.g. it is not like it has been running for 5 years
< vasild>
But I will look into that anyway, it is good to compare different solutions and pick the "better" one
< BlueMatt>
:q
< sipa>
:q!
< dongcarl>
To the point of Guix continually making progress on bootatrapping, live-bootstrap maintainer mentioned yesterday that: live-bootstrap can build all of the guix bootstrap binaries on i386 (including static guile) so we have an option of replacing bootstrap binaries with built in live-bootstrap.
< dongcarl>
We shall see if it can link up and be used to further minimize the bootstrap chain
< roconnor>
dongcarl: what is guix time-machine? The man page is useless in typical GNU fashion.
< vasild>
If all (most?) nodes are already running master + #22112 (merged 3 days on on Jul 13)... that would mean I2P peers now advertise themselves with port 0 and ports have been changed in people's addrmans. If that is the case, then ResetI2PPorts() has achieved its purpose and can be removed. <-- jonatack, sipa, jnewbery, laanwj
< jonatack>
jnewbery: have you (a) been testing I2P on master, and (b) found cases of addrman corruption? IIUC the ossfuzzer is naturally turning up cases we didn't know about before. ISTM that is to be expected.
< fjahr>
hi
< achow101>
hi
< jonatack>
hi :)
< michaelfolkson>
hi
< michaelfolkson>
I have some general Taproot roadmap questions if there's nothing on the agenda
< kvaciral[m]>
hi
< meshcollider>
No more urgent wallet things found that need to go into 0.22 right?
< meshcollider>
22*
< kanzure>
hi
< achow101>
#22461 is the only wallet pr marked for 22.0
< michaelfolkson>
PSBT won't be supporting with MuSig. But to support Taproot multisig (CHECKSIGADD not MuSig) does this get included in PSBT spec?
< achow101>
anything script based is supported by psbt
< sipa>
PSBT doesn't need support for individual script types; that's up to individual singer implementations
< sipa>
taproot support in general needs extensions (already proposed), and MuSig will need its own ones
< sipa>
but scripts don't need anything
< sipa>
*signer, not singer
< michaelfolkson>
I guess I'm trying to unravel what is specs and what is the Core wallet. "Anything script based" but the Core wallet doesn't support Taproot scripts
< achow101>
the psbt fields have fields for the leaft scripts
< sipa>
as i said, that's up to the individual signer
< achow101>
leaf scripts can be anything
< sipa>
clearly you can't expect every signer to support every script
< achow101>
it's up to the implementations to figure out what those scripts are doing, and what to do with them
< michaelfolkson>
Ok I think I understand, thanks
< michaelfolkson>
The PSBT part isn't doing much
< sipa>
it's just an interchange format
< gene>
do PSBT implementations need to validate scripts?
< sipa>
and the Bitcoin Core wallet does support taproot scripts by the way, just only very limited ones (effectively just "<pubkey> OP_CHECKSIG")
< achow101>
gene: PSBT implementation don't have to do anything with scripts. whether they should or should not depends on their role
< sipa>
signers may want to check any number of properties of what they're signing before deciding that signing is right
< sipa>
but what that is, is up to them
< sipa>
PSBT just makes sure that data can be made available to them
< gene>
achow101: so the PSBT impl can just treat scripts as opaque blobs, and then what's on the other side (e.g. a HW wallet) decides what to do with script?
< achow101>
gene: yes, although both sides would be PSBT implementations, just different roles
< gene>
+1
< sipa>
it's a bit ambiguous what you call "PSBT impl"
< sipa>
if you're just referring to the serializer/deserializer, yes, definitely
< sipa>
but you probably mean "any software that interacts with PSBT", and they may or may not want to implement various checks, depending on their role and what things they support
< gene>
yeah, the software/firmware that handles PSBT serialization
< gene>
ok, that makes sense
< sipa>
most hardware wallets don't actually handle PSBT directly - they have a software driver that converts between PSBT and the HW API calls
< michaelfolkson>
Ok I will reread PSBT BIP with that understanding. That helps, thanks
< achow101>
in terms of Core supporting more complicated taproot scripts, I think that will depend on miniscript (and more generally, descriptors)
< michaelfolkson>
I was surprised that he thinks it is likely Miniscript gets merged into Core without supporting Taproot but I (kinda) understand the argument
< sipa>
the goal is obviously to extend it at some point to support taproot
< sipa>
but i don't think it's worth doing more spec work if the implementation isn't making progress
< michaelfolkson>
To summarize the Taproot Miniscript syntax is easy to design but this will likely be supported by rust-miniscript before the C++ implementation
< sipa>
it depends what people work on
< sipa>
i don't know if anyone working on rust-miniscript is working on this either, but they may be in a better position to start doing so
< michaelfolkson>
"implementation isn't making progress" <- you mean the C++ implementation of Miniscript here right?
< sipa>
yes
< michaelfolkson>
But there is this stepping stone between just supporting single key Taproot spends and Miniscript generic scripts
< meshcollider>
I think we should definitely prioritise it after 22 is done
< michaelfolkson>
Descriptor scripts could be supported without Miniscript
< sipa>
they are
< sipa>
they're just very minimal right now
< sipa>
we can add support for OP_CHECKSIGADD based scripts to descriptors and signing logic for them
< michaelfolkson>
Taproot descriptor scripts beyond just Taproot single pubkey spends I mean
< sipa>
outside of miniscript
< sipa>
michaelfolkson: again **they are**
< sipa>
read the spec
< michaelfolkson>
I'm speaking from Core wallet perspective here
< sipa>
just only <pubkey> OP_CHECKSIG ones
< sipa>
we're not restricted to inner key only taproot
< michaelfolkson>
So I think my statement is correct. Have to be clearer on when taking about the Core wallet and when talking about specs
< sipa>
you can construct 1-of-n using what is supported today, for example
< sipa>
i'm only talking about core as well
< michaelfolkson>
Hmm ok, that is new information to me. I thought it was just 1-of-1 for key path and multiple script paths
< michaelfolkson>
With 1-of-1 in each leaf
< sipa>
yes, exactly
< sipa>
that's 1-of-n
< achow101>
michaelfolkson: each leaf is "1-of-1", but you can have as many leaves as you want
< michaelfolkson>
Right, I knew that
< michaelfolkson>
But not 2-of-3 or m-of-n
< michaelfolkson>
m>1
< meshcollider>
No, there's no support for threshold
< michaelfolkson>
That needs CHECKSIGADD or MuSig
< sipa>
or FROST, or other native threshold scheme
< michaelfolkson>
Right
< michaelfolkson>
But we can do thresholds without Miniscript
< michaelfolkson>
So that is probably next step
< michaelfolkson>
(for the wallet)
< michaelfolkson>
Doing as much as possible without needing generic scripts that would need Miniscript
< meshcollider>
Why? Why not just focus on getting miniscript in and then go from there?
< achow101>
I would actually prefer to add miniscript so that we don't have any incompatibilities and stop special casing things like multisig
< meshcollider>
Exactly ^
< michaelfolkson>
meshcollider: I'm just trying to read into sipa comments (or at least what he was thinking on that Miniscript issue)
< sipa>
it'd be great to just integrate miniscript
< michaelfolkson>
To add Miniscript that needs updating the C++ implementation for Taproot
< sipa>
if people would want to work on that :)
< michaelfolkson>
Are there difficulties ensuring the C++ implementation and the Rust implementation are equivalent?
< michaelfolkson>
(for Miniscript)
< sipa>
when we first wrote it, we did comparison testing
< sipa>
if integrated into bitcoin core, it'd be also easy to e.g. produce test sets using fuzzing, which could be tested in other implementations
< sipa>
or vice verse
< michaelfolkson>
"I'd very much want to see it further along in integrating into Bitcoin Core" <- That sounded to me like you'd rather Miniscript was merged into Core before it supported Taproot
< sipa>
yes
< sipa>
but no reason why both can't happen if people work on it
< michaelfolkson>
Do others agree with that viewpoint e.g. achow101 meshcollider? Miniscript into Core first before it is updated to support Taproot?
< achow101>
It'd probably be easier to implement taproot into the c++ implementation once it in core
< gene>
having a formal verification of miniscript (far future) would also allow mechanically verifying different implementations
< meshcollider>
Like sipa said earlier, there's no point working on taproot support for it if it isn't going to get merged anyway, so better to focus on getting it merged first
< michaelfolkson>
Cool, makes sense
< sipa>
the miniscript c++ repository is currently somewhat in limbo; it works, but it heavily depends on code copied from bitcoin core
< sipa>
and testing it is effectively dependent on being able to use bitcoin core's scripting engine
< sipa>
so it's not very appealing to see them diverge further if more feature are added to miniscript, independently of bitcoin core's script code
< michaelfolkson>
I see
< michaelfolkson>
Ok so that should be the focus (if we want the Core wallet to support generic Taproot scripts which would be cool/ideal)
< michaelfolkson>
*eventually support
< michaelfolkson>
Ok that's all my questions. Thanks all
< dongcarl>
roconnor: Oh, `guix time-machine --commit=abcdef -- build coreutils` means build guix at commit abcdef and run that older guix with the arguments ["build", "coreutils"]