< sipa>
there is probably a place for them, like goto
< sipa>
but almost always, they just lead to badly designed abstractions
< sipa>
a clear design should have code that operates outside the critical section, and code that operates inside
< sipa>
but not code that works in both
< gwillen>
my impression is that the usual better approach is to have Foo() which calls Foo_locked(), and callers who already hold the mutex can call the latter directly
< sipa>
indeed
< jeremyrubin>
I agree with this in general, but am trying to understand the specific issues we're solving in the mempool rather than general API design
< jeremyrubin>
It seems to get rid of recursive mutex in favor of our own implementation of it
< jeremyrubin>
which seems worse
< sipa>
oh
< jeremyrubin>
So I'm trying to understand hebasto's motive in the PR to be able to provide constructive feedback :)
< sipa>
i have not looked at the code
< sipa>
eh, i agree - that change isn't a step in the right direction
< bitcoin-git>
[bitcoin] andrewtoth closed pull request #18941: validation: Persist coins cache to disk and load on startup (master...persist-coinscache) https://github.com/bitcoin/bitcoin/pull/18941
< luke-jr>
sipa: recursive mutexes just guarantees that <this> code always operates inside, no matter the scope of the lock
< luke-jr>
gwillen: what good is adding a wrapper for every method that just takes a lock before calling the real one?
< sipa>
luke-jr: making the separation between inside/outside critical section clear
< jeremyrubin>
I think recursive mutexs and lock annotations have a slight redundancy
< jeremyrubin>
run time v.s. compile time lock checking
< jeremyrubin>
run time should be strictly more flexible, but also permits some undesirable behaviors
< sipa>
(non-recursive mutexes are also slightly more efficient, but not enough to justify changing things just because of that imo)
< gwillen>
luke-jr: recursive mutexes are often used when people want to call the same method while sometimes holding the lock it wants, and sometimes not
< gwillen>
a better approach is to have two methods, and have them call the correct one
< luke-jr>
sure, the lock annotations are a reasonable alternative to recursive mutex - but I don't think a wrapper for every method just to add a lock is reasonable
< sipa>
luke-jr: most methods are only called in one context
< luke-jr>
sipa: not all though
< sipa>
fwiw, addrman has this design
< gwillen>
right, the point is that you only need a wrapper for methods which are called in both contexts, not every method
< sipa>
it has all code running inside the cs in the .cpp file
< luke-jr>
gwillen: some abstractions would have it for most methods
< gwillen>
it is relatively rare (and probably sometimes a code smell) to need this
< sipa>
and then all publicly accessible code grabs the lock, and calls the internals
< gwillen>
but if you do need it, having two methods is better than using recursive mutexes, the existence of which is a mistake
< gwillen>
(instead of having two methods, you can also take a parameter indicating whether the lock is held or not, which is how some of the code under discussion does it, but I disprefer that approach)
< luke-jr>
the benefit of annotations is that you force the caller to be aware it's using a lock
< luke-jr>
you lose that with wrappers
< jeremyrubin>
Everyone enjoy your evenings ;)
< sipa>
gwillen: agree
< gwillen>
wrappers and annotations are not mutually exclusive, you should definitely still use annotations
< luke-jr>
you can't use annotations with a wrapper..
< sipa>
luke-jr: huh?
< luke-jr>
(unless it's a recursive mutex)
< jeremyrubin>
I didn't realize this would be such a *contentious* topic
< sipa>
you'd have the internal function annotated with require_lock, and and the wrapper with a lock_excluded annotation
< gwillen>
is this a feature of the particular annotations that happen to be available in the bitcoin core codebase? It's certainly not a feature of annotations in general.
< luke-jr>
sipa: okay, but that defeats the point
< sipa>
luke-jr: i don't see how?
< sipa>
this is my preferred approach
< luke-jr>
sipa: the code calling the wrapper no longer self-documents/makes the developer aware of using the lock
< sipa>
external code should only be seeing the wrapper
< sipa>
internal code should only ever call the non-wrapped function
< sipa>
thus providing a perfectly clear separation between the two
< luke-jr>
and what if the external code needs to hold the lock longer than just the call?
< sipa>
rewrite the code :p
< jeremyrubin>
luke-jr: use an external lock?
< sipa>
(i know that's not always possible - but in many cases it is)
< luke-jr>
eg, with a wallet, it's pretty reasonable to want atomic operations
< luke-jr>
doing multiple things to the wallet with the lock held
< luke-jr>
you *could* just move the entire logic into the wallet, but that's a bad design in some cases
< sipa>
luke-jr: yeah, that's a good example of an exception
< gwillen>
btw jeremyrubin it's not switching to a homebrewed mutex, if you chase down the types it's switching from AnnotatedMixin<std::recursive_mutex> to AnnotatedMixin<std::mutex>, which seems like the right thing
< sipa>
gwillen: i think it's referring to the "if locked then unlock else assertlocknotheld" pattern that i see some times
< jeremyrubin>
sipa: gwillen: yes, that's what I meant
< sipa>
luke-jr: i think a reasonable approach for that may be having a publicly visible lock for synchronizing a consistent view, which is distinct from the internal wallet's lock (which protects internal data structure correctness, not externally visible consistency)
< gwillen>
well, adding an argument for whether the mutex is held is equivalent to a wrapper, just uglier, which I think is what's going on here
< gwillen>
I dislike it because it means you are not using RAII
< sipa>
gwillen: indeed
< gwillen>
and if you take an exception the world ends
< jeremyrubin>
I think it's basically getting rid of a recursive mutex for code that's still designed to take a recursive mutex
< gwillen>
it's better than a true recursive mutex because it's not possible to recurse by accident, you have to declare at call time which behavior you want (although better if you had to declare statically at compile time)
< sipa>
it looks like that
< jeremyrubin>
The correct refactor would be to make the code not do anything fancy with locks, or to just leave it
< jeremyrubin>
gwillen: I think the chances of a bug or error in custom logic is higher than a recursive mutex
< jeremyrubin>
and accidental recursion would be a bug lock or no
< gwillen>
(sorry I mean, accidental mutex recursion, that is, calling a function while holding a mutex, not expecting the callee to also lock it, resulting in the callee violating the caller's invariants)
< gwillen>
(this is the fundamental problem of recursive mutexes)
< gwillen>
(and I assume the motivation behind hebasto's refactor)
< jeremyrubin>
Ah sure. But the code in the callee could also just not lock at all and modify the protected variables?
< * jeremyrubin>
pines for rust
< gwillen>
but that would be an obvious bug in the callee, whereas with recursive mutexes you can have a caller and a callee that both appear to be correct by local inspection (they take a mutex while modifying the protected variables)
< gwillen>
but if the caller calls the callee in the middle of doing so, the result can violate the mutex invariant surprisingly
< jeremyrubin>
would it be? it depends on how pervasive locking annotations are in your code
< jeremyrubin>
It's entirely reasonable to write a helper which doesn't assert which locks it expects
< gwillen>
Well, it would be nice if it weren't. :-)
< * jeremyrubin>
pines for rust
< luke-jr>
gwillen: it'd be nice if C/C++ added non-scoping conditionals :P
< gwillen>
anyway that's the story of the theoretical basis for getting rid of recursive mutexes, as I understand it... but adding annotations is probably a higher priority, yeah.
< luke-jr>
jeremyrubin: Rust needs to fix its problems with bootstrap and shared libraries
< jeremyrubin>
c++ needs to fix... everything else basically :p
< * sipa>
grabs popcorn
< luke-jr>
C++ isn't broken, even if it doesn't provide features some might idealise
< jeremyrubin>
anyways bootsrap stuff I think is more in dongcarl's weelhouse so I'll tag out for him ;)
< luke-jr>
IMO it's still in Rust developers' court; they haven't made it practical
< luke-jr>
and frankly seem to be actively trying to make it difficult
< gwillen>
my understanding is that bootstrapping from a C compiler using mrustc is annoying but functional, and guix has this working
< gwillen>
it looks to me like they've gotten better, mrustc can build rust 1.29 now, so the entire chain of rustc-rustc bootstrapping from that page is outdated
< gwillen>
I don't know whether guix takes advantage of this, though, I have not used it
< fanquake>
I'd also like to know what has "gotten worse"
< luke-jr>
fanquake: Rust seems to make no effort to be backward compatible, so ~every release adds another step you have to compile
< luke-jr>
eg, you can't just mrustc a stable rustc, then use that to get the latest rustc
< gwillen>
in fact, assuming it still works, mrustc appears to ship a shell script that demos bootstrapping from a C compiler through mrustc and rustc 1.29.0 to rustc 1.30.0 (it stops there, but it's a proof of concept that you can build later versions)
< gwillen>
I assume you do not actually have to crawl one version at a time, but it would be nice to have a compatibility matrix to see how many compiles it takes
< luke-jr>
it would be nice if Rust devs stopped breaking the language compatibility :p
< gwillen>
if you just wnat a proof of concept, probably easier to do that than to work out the full matrix
< gwillen>
oh, I do like the point made in the README here, which is that once there is full reproducibility, you don't need to do this every time, you just need to do it once and check that the hash of the result matches the official build
< phantomcircuit>
luke-jr, the issue of doing atomic things like that is more or less analogous to database consistency issues
< phantomcircuit>
no real solution here just a thought
< sipa>
i was curious what the bootstrapping cycle for modern GCC is like
< sipa>
potentially with GCC 4.7 in between (unless how good C++98 support in GCC 3.3 was)
< gwillen>
I assume C/C++ (as implementd in GCC) has changed substantially in that time, so the real issue is not so much language change, as the compiler being overly aggressive in taking advantage of the latest features of the language
< gwillen>
which GCC is presumably fairly careful to avoid, if you can really bootstrap it that directly
< sipa>
yeah, GCC switched to C89 in GCC 3.4, to C++98 in GCC 4.8, to C++11 in GCC 11
< sipa>
they're slower in adopting language changes than bitcoin core :p
< fanquake>
heh
< sipa>
though, that's an unfair comparison with rust, which just changes much more rapidly by virtue of being younger
< fanquake>
The Guix bootstrap seed is now even smaller
< fanquake>
Targeting another 50% reduction during the next round of work
< sipa>
nice
< bitcoin-git>
[bitcoin] jamesgmorgan opened pull request #19317: Add a left-justified width field to log2_work component for a uniform debug.log output (master...jmorgan-updatetipfmt) https://github.com/bitcoin/bitcoin/pull/19317
< luke-jr>
sipa: compilers *should* be very slow with adopting new features
< luke-jr>
for anything else, you can always just upgrade the compiler so long as the new compiler builds
< dburkett>
A little late, but one "self-documenting" RAII way of handling locks is taking in a lock reference like: `void func_locked(const std::unique_lock<std::mutex>& lock)`
< dburkett>
It's not as easy to misuse, and doesn't require relying on non-compiler-enforced annotations. The only shortfall is you still need to document which mutex must be locked in cases where there are multiple.
< dburkett>
You could even solve that by defining a unique type for each mutex or something, but that's going a little overboard.
< aj>
dburkett: having a unique type for each mutex would probably work fine for globals/module-level mutexes, but not for mutexes that are members of an object. could also do an AssertLockHeld(mutex,guard); that checks the guard corresponds to the mutex and isn't random garbage
< dburkett>
AssertLockHeld is only runtime enforceable, but yes that's an improvement.
< dburkett>
The mere existence of a lock parameter at all though is an improvement over the current situation. Any assertions/annotations/custom mutexes beyond that are a bonus
< bitcoin-git>
bitcoin/master 80d4423 Troy Giorshev: Test buffered valid message
< bitcoin-git>
bitcoin/master 343c0bf MarcoFalke: Merge #19304: test: Check that message sends successfully when header is s...
< bitcoin-git>
[bitcoin] MarcoFalke merged pull request #19304: test: Check that message sends successfully when header is split across two buffers (master...2020-06-test-partial) https://github.com/bitcoin/bitcoin/pull/19304
< bitcoin-git>
[bitcoin] fanquake opened pull request #19318: build: disable -stack-clash-protection on Windows (master...disable_stack_clash_windows) https://github.com/bitcoin/bitcoin/pull/19318
< achow101>
wumpus: yes
< wumpus>
achow101: ok, replaced
< achow101>
although #19292, #19308, and #19310 shouldn't conflict with each other so they can be merged in any order
< gribble>
https://github.com/bitcoin/bitcoin/issues/19292 | wallet: Refactor BerkeleyBatch Read, Write, Erase, and Exists functions into non-template functions by achow101 · Pull Request #19292 · bitcoin/bitcoin · GitHub
< wumpus>
achow101: true, I do think that it's better to have a smaller PR on there if you have split up things anyway
< achow101>
next in line is 19308
< wumpus>
ok, I'll leave it to someone else to replace that, I'm done for it for a bit :)
< achow101>
i'll wait for the meeting
< wumpus>
nah already swapped it
< bitcoin-git>
[bitcoin] MarcoFalke opened pull request #19320: wallet: Replace CDataStream& with Span<char> where possible (master...2006-walletSpan) https://github.com/bitcoin/bitcoin/pull/19320
< wumpus>
MarcoFalke: re: #19033, you added the "waiting for author" tag, what is it waiting on?
< wumpus>
let's remove it from high priority for review for now, then
< wumpus>
doesn't make sense for people to review it if it's broken :)
< wumpus>
okay, yes I see why now
< wumpus>
it's unfortunate that it turns out to be so difficult to get the http shutdown correct, I remember lots of times it was 'fixed', I wish libevent-http just came with a multi-threaded web server itself instead of us having to hack it on top of it
< promag>
wumpus: re HP, sure
< promag>
wumpus: there's also corner cases regarding shutdown and ongoing rpc responses
< wumpus>
yes, ther's always been
< promag>
and also not being able to discard lots of incoming requests
< wumpus>
or at the least, defer accept()
< wumpus>
but no it accepts every connection immediately without some kind of fd quota
< wumpus>
well, switching to it from boost::asio seemed like a good idea at the time (it at least solved some issues)
< wumpus>
I'll have a look at it too some time
< luke-jr>
#11082 rebased yet again
< gribble>
https://github.com/bitcoin/bitcoin/issues/11082 | Add new bitcoin_rw.conf file that is used for settings modified by this software itself by luke-jr · Pull Request #11082 · bitcoin/bitcoin · GitHub
< bitcoin-git>
bitcoin/master 25f3554 Hennadii Stepanov: scripted-diff: Make SeparatorStyle a scoped enum
< bitcoin-git>
bitcoin/master 0865a88 MarcoFalke: Merge bitcoin-core/gui#3: scripted-diff: Make SeparatorStyle a scoped enum...
< MarcoFalke>
Just merged the first pull from the gui repository. Hope nothing caught fire
< achow101>
oh noes my computer is on fire
< troygiorshev>
does anyone have experience with our Optional? I'm looking to use it with a CNetMessage but I'm worried about the performance. Looks like it doesn't have move semantics until 1.56.0. Has anyone run into this before?
< provoostenator>
So how does Gribble link to GUI issues now?
< luke-jr>
hrm, might be annoying to have a different # namespace :/
< provoostenator>
Maybe prefix it? E.g. #gui-2
< MarcoFalke>
In github the normalized and absolute identifier is "bitcoin-core/gui#3" or "bitcoin/bitcoin#3"
< luke-jr>
someone should fix gribble for whatever we do :P
< provoostenator>
Where does Gribble's source live?
< luke-jr>
nanotube's repo on gitlab iirc
< dongcarl>
Looks like I have missed a fun, popcorn-worthy convo above, just to add what I know: the mrustc support is solid, and development of it is very active in Guix, and just 6 days after the release of mrustc 0.9 (equiv of rustc 1.29.0), patches were being reviewed for taking advantage of the newer version and shortening the bootstrap chain by 10 steps
< dongcarl>
(1.19.0...1.29.0).
< dongcarl>
The current method crawls through one minor version at a time (which is probably the safe way to do it), and there's a substantial number of rust package users in Guix who will notice if the bootstrap chain breaks.
< cfields>
luke-jr: gah, sorry, forgot about that one. Will take a look regardless of how it's tagged.
< luke-jr>
cfields: thanks
< jonasschnelli>
hi
< jamesob>
tangential to 19028, maybe we should set logthreadnames=1 by default if we can show there isn't a performance hit
< luke-jr>
I did notice #18818 was insufficient for Knots, but only because Knots is distributing files rendered from SVGs at dist-time - shouldn't affect Core's needs
< wumpus>
no strong opinion, but I'm not sure the thread names are useful to most non-developers
< wumpus>
for running the tests it makes sense though
< jamesob>
I'm thinking it could be useful when we ask for debug.logs in bug reports
< wumpus>
but we had a similar discussion at the time about adding microseconds by default to the log - like okay, for some things it might be useful, but it just widens the log messages
< dongcarl>
luke-jr: I think after exploring a bit in the `tarfiles` python module (quite powerful, and shipped with python by default), we can use it to "union" the `make dist` archive, and the `git-archive` archives, happy for yours to be merged in first, just wanted to mention it here.
< luke-jr>
dongcarl: GNU tar can do it too, but I'm not sure it's worth the complexity
< wumpus>
jamesob: can you show a specific example where it helps? are there many log messages that are ambigious as to where they originate?
< wumpus>
(especially of those enabled by default)
< jamesob>
wumpus: fair point, will raise again if I can
< wumpus>
jamesob: thanks
< dongcarl>
luke-jr: last time I tried it with GNU tar I remember it had weird behaviour, anyway, I'll shut up until I have code :-)
< luke-jr>
dongcarl: also, I had originally made 18818 to do that, but I was uncertain of the ramifications of having different timestamps for the modified files
< luke-jr>
(iirc, git-archive is using a timestamp potentially after the gitian reference timestamp, so the source files would appear "newer" possibly)
< jnewbery>
I don't see any downside to logging thread names by default (unless there's a performance hit). It does make tracing what's going on in the log files a lot easier
< wumpus>
jnewbery: do you have any specific examples where it wouldh elp or would have helped?
< dongcarl>
luke-jr: true, but with the `tarfiles` python module we can make decisions on that programmatically ourselves, instead of GNU tar doing what its default conflict resolution is
< wumpus>
the default amount of logging shouldn't be a performance hit in any way, adding a field isn't going to make it noticably worse
< wumpus>
so I'm more concerned about making the log unneceessarily spammy/verbose than performance hit here
< gwillen>
thread names are non-unique, right? it might be nice to log thread IDs in addition or instead.
< gwillen>
otherwise all the worker threads will log the same name and it still won't help to tell them apart.
< wumpus>
all the names are unique
< jamesob>
gwillen: I think they're unique at the moment; those that aren't have an increasing suffix
< gwillen>
ah okay great, nevermind
< jnewbery>
wumpus: it's just easier when you're eyeballing the log to get an idea of what's going on. Obviously if you know or look up the location of every log call in the source, you can work it out.
< sipa>
i just git grep the log message :)
< wumpus>
sipa: same, it's the only way to be sure :) log messages tend to be unique enough
< sipa>
(no objection to logging thread names by default if it has no performance impact, though)
< provoostenator>
(hi)
< wumpus>
but if everyone wants to add tghat field and I'm the only one slightly sceptical about it, just add it, no strong opinion
< sipa>
let's first benchmark?
< jnewbery>
validation interface logging is also super helpful if you haven't played around with it. It's nice to see when the signals being added to the callback queue and when they're serviced by the scheduler thread
< wumpus>
if looking up a thread name really causes a performance hit something is really wrong btw
< wumpus>
if logging is on the performance critical path (with the default amount of logging) in the first place
< sipa>
agree, but haven't we had surprising experiencing with this in the past?
< jamesob>
wumpus: yeah I reckon you're right. couldn't hurt to see if something's really wrong though
< jamesob>
MarcoFalke has proposed to add a logging benchmark re: the previous issue, which I think is still open?
< sipa>
make it it isn't looking up the thread name for log categories that are disbaled
< wumpus>
sipa: there was some worry about enabling TLS causing a performance hit (independent of whether it was actually used frequently)
< wumpus>
sipa: but this turned out not to be the case
< wumpus>
(TLS as in Thread Local Storage, not the other thing)
< MarcoFalke>
jup the log bench is still open
< MarcoFalke>
imo it can be merged. The risk should be zero
< wumpus>
I implemented the thread name lookup using a map but then it turned out to not be the problem at all
< jamesob>
MarcoFalke: agree, think I've acked it
< sipa>
ok
< wumpus>
in any case please do not log on the critical path
< wumpus>
definitely not by default (with debug flags is fine)
< wumpus>
but if it's logging in an inner loop or something that really affects, say, validation performance, that's not how logigng should be used
< wumpus>
it's why I find a logging benchmark kind of weird, we're not trying to optimize logging throughput
< jamesob>
it's less for optimization and more just an assurance we're not doing anything totally dumb
< jnewbery>
jamesob: agree
< MarcoFalke>
the benchmark also checks that *disabled* logs don't affect performance
< MarcoFalke>
maybe I should call is nolog bench
< MarcoFalke>
*it
< wumpus>
that's a good idea
< wumpus>
logprintf arguments shouldn't even be evaluated in that case
< MarcoFalke>
jup (I broke that once)
< * MarcoFalke>
hides
< wumpus>
heh
< wumpus>
#topic Taproot implementation plan (moneyball)
< moneyball>
Hi everyone, I wanted to check in here to get a sense for whether contributors are imagining the taproot implementation making it into v.21 or not. If yes, then it is likely the case that there needs to be pretty extreme focus in order to make it in time.
< jeremyrubin>
yeah I think we don't usually aim for major releases to have forks, should be a minor
< jeremyrubin>
So there really isn't any release window time crunch to push for
< jeremyrubin>
But I agree in principal with trying to figure out a path for activation
< moneyball>
luke-jr, sipa: well there is including the activation code and then separately configuring the activation parameters. right?
< sipa>
moneyball: depends on the activation mechanism
< sipa>
if a new activation mechanism needs code, then yes that needs code :)
< moneyball>
ha :)
< jeremyrubin>
One thing I'm curious to look at is if the recent changes to the sighash & the recent hardware wallet issues are informing or suggesting any other sighash changes we should be doing concurrently.
< moneyball>
ok my understanding of that is that a new activation method is planned to be proposed to the mailing list, and IF there is consensus around that, then yes, that code would need to be added to Core
< ariard>
haven't reviewed yet 17977 yet, what's the test coverage of it ? do we need to add more support for testing taproot tree composition in fuzzing or test framework?
< sipa>
ariard: the python code is effectively an extensive generate-random cases with lots of edge cases, and compare the python-created signatures against block/tx validation
< sipa>
ariard: more testing is absolutely welcome of course
< jeremyrubin>
moneyball: fwiw I've talked with a bunch of contributors and I think Modern Soft Fork Activation is far from a universally loved approach. That conversation should probably be had more exentsively before you hitch taproot onto that wagon
< sipa>
moneyball: that sounds like it needs ML discussion first
< moneyball>
luke-jr: ok i guess i understand that signet is not required per se, but, some kind of test plan would be. has there been much discussion and consensus for how to test this in Core?
< moneyball>
sipa: yes
< sipa>
it's hard to ask people here what they think about an approach that hasn't even been published yet
< moneyball>
my hope was to focus on non-activation method work needed in Core
< sipa>
yeah, that makes sense
< moneyball>
perhaps it was a mistake having line item one in that doc
< provoostenator>
Although not required, it would be really nice to have a Taproot Signet.
< sipa>
i think implementation wise that list pretty much covers it
< moneyball>
ok i just deleted that line item from the doc :)
< moneyball>
i would love to hear more discussion about testing approach. what is there general agreement on? what are open questions that need to be discussed?
< jeremyrubin>
I think that running on signet doesn't really do anything by itself
< luke-jr>
provoostenator: sure, I was just answering moneyball's request for things not required :P
< jeremyrubin>
The real challenge is to get integration tests somewhere
< provoostenator>
moneyball: having a signet explorer somewhere can help with testing too
< jeremyrubin>
E.g., people attempting to integrate it and acutally use taproot
< jeremyrubin>
I would put more stock in, e.g., an LND fork with taproot support against regtest than signet (but signet would be great too)
< sipa>
jeremyrubin: that would be great, but i fear that it's a bit of a chicken and egg problem
< luke-jr>
segnet worked okay AFAIR
< luke-jr>
testing should be before merge anyway
< provoostenator>
luke-jr: signet could be in a release and completely changed in the next release though
< ariard>
sipa: does feature_taproot.py attempt any coverage-guided like a fuzzer?
< sipa>
yeah, if not signet we can create a (normal) testnet with it activated too (i think signet would be preferable, but if it somehow doesn't make it in time, i don't think that would be a blocker)
< sipa>
ariard: no
< provoostenator>
Or we could release a taproot signet binary seperately
< jeremyrubin>
sipa: indeed it's hard. I think if signet comes out then people will integrate test against it
< luke-jr>
provoostenator: that seems like a given
< jeremyrubin>
Just more noting that just getting signet out doesn't do anything in terms of progress alone
< luke-jr>
provoostenator: otherwise we'd be merging taproot before testing it
< sipa>
ariard: fuzzing definitely makes sense to test for things like memory violations and UB
< wumpus>
well if it helps getting more attention to testing taproot, that's progress
< sipa>
luke-jr: i think there are different stages of testing, and different stages of getting attention to it
< ariard>
sipa: right you may have nast edges cases we wide trees and oversized tapscripts?
< sipa>
ariard: there is a test for the max depth of the tree, if that's what you're asking for
< sipa>
luke-jr: and at some point it will need to be merged for people to test against a kind of testnet, which hopefully informs discussions on activation
< ariard>
okay great
< sipa>
it can't be testnet/signet tested before being merged - but different kinds of testing are obviously necessary before that point
< jeremyrubin>
I think the issue with signet is it doesn't add a new message type/storage place for the signatures
< jeremyrubin>
I understand why kallewoof did it that way and it makes sense
< jeremyrubin>
But it just makes it difficult for people to want to merge it
< sipa>
jeremyrubin: this seems orthogonal
< jeremyrubin>
slightly
< sipa>
i don't think taproot should be blocked by signet in any case
< luke-jr>
sipa: but I think we want tapnet before merging?
< jonatack>
hi... fwiw MarcoFalke, fjahr, brakmic and I were testing signet for some time and going back and forth with kallewoof on improvements... iirc it's the PR is in pretty good shape
< sipa>
luke-jr: perhaps
< sipa>
luke-jr: i think that may make sense
< jeremyrubin>
What about just flag daying testnet?
< sipa>
there aren't any associated P2P changes, so i think the need for that level of testing may be lower than with segwit
< wumpus>
as said, testnet needs to be compatible between releases, so there's not much scope for experimentation there
< * luke-jr>
glad to hear brakmic hasn't given up on us completely :x
< luke-jr>
wumpus: doesn't need to be..
< ariard>
do we expect to introduce new standard rules on taproot witness?
< wumpus>
I mean, I think there should be a flag day on testnet before considering activation on mainnet, but only after the protocol and implementation is virtually finalized
< MarcoFalke>
lol, wasn't testnet hardforked for segwit?
< MarcoFalke>
I mean silent hardfork. "hardfork" is probably the wrong word
< jeremyrubin>
wumpus: ah I see. I thought we can just reset testnet if we want. Does anyone care?
< jeremyrubin>
wumpus: you can also make a soft fork flag day that a rule is enforced for N blocks only
< wumpus>
jeremyrubin: it's possible but should probably be avoided
< sipa>
ariard: yes, though they're pretty weak; upgradability (annex, new leaf versions, ...), and max stack item size
< sipa>
moneyball: i believe sdaftuar has some thoughts on that
< ariard>
sipa: indeed all of them are constraints on new data structure so no risk to tamper with network/break existent applications
< jeremyrubin>
network stuff isn't required
< jeremyrubin>
It can be done after
< sipa>
jeremyrubin: read the wtxid relay PR
< sipa>
it gives a justification
< ariard>
right because v0 segwit nodes are going to waste bandwidth constantly redownloading taproot txn they can't verify
< sipa>
indeed
< sipa>
this depends on how upgraded nodes are at the time of activation of course
< sipa>
so it may not be a big issue, but having a way to reduce that impact beforehand sounds like an improvement
< ariard>
ofc how long it takes to get 80% of segwit nodes ? or similar number based on previous forks?
< jeremyrubin>
yeah I am familiar. It's not great, but I personally don't think it's blocking
< sipa>
ok
< jeremyrubin>
I could be wrong on that though
< sipa>
moneyball: i think wtxid could be done before 19184
< sipa>
(but i'm obviously biased in liking 19184 to get in)
< ariard>
jeremyrubin: don't you have a bad effect as we see more taproot txn ande nodes relaying them the cost is increasing non-linearly for non-upgraded nodes?
< wumpus>
meeting time about to end
< sipa>
ariard: well, it's linear, but with a possibly big constant factor
< moneyball>
ok thank you for the feedback. this has been valuable. lots to follow-up on though!
< wumpus>
#endmeeting
< lightningbot>
Meeting ended Thu Jun 18 20:00:18 2020 UTC. Information about MeetBot at http://wiki.debian.org/MeetBot . (v 0.1.4)
< jeremyrubin>
I'll retract on 18044 being required; I was quibbling over whether or not we could release without it or whether we should. We should definitely release with a fix, I just don't think it's required insofar as it's a blocker for e.g. testnet
< hebasto>
MarcoFalke: could you suggest a comment for #19323?
< achow101>
dongcarl: the current repo owner needs to initiaite it. then a bitcoin-core admin needs to approve it
< dongcarl>
I see
< dongcarl>
cfields: Could you initiate it?
< achow101>
oh and the repo owner needs to have create permissions in the bitcoin-core org
< achow101>
forgot the most important part
< achow101>
when I moved HWI, wumpus had to grant some permission to all members, then remove that permission
< dongcarl>
:-/
< dongcarl>
Seems easier just to start afresh and push to the repo?
< achow101>
then you lose any existing issues and PRs
< cfields>
It's just 1 file :p
< achow101>
maybe just add it to the packaging repo then? it's kind of packaging related
< cfields>
I have absolutely no preference. I only created a repo for it because I hoped someone better with python would take it and rewrite it, heh.
< dongcarl>
achow101: I think the hope is that other projects that cross-compile for mac might find this useful, as it does the job of 2 separate programs in one python script
< cfields>
I'll just initiate the xfer. Let me try to figure that out.
< sipa>
can i help?
< cfields>
"You don’t have the permission to create public repositories on bitcoin-core"
< cfields>
Yeah, let's not worry about moving it. It's not worth the effort.
< cfields>
Either new repo or packaging repo are fine.
< cfields>
sipa: can you create a new repo in bitcoin-core ?
< sipa>
yes
< cfields>
sipa: eh on second thought, maybe best not to create some weird fork history. I'll just invite you as an owner of my repo and you can initiate the move?
< sipa>
sgtm
< sipa>
cfields: i don't have ownership rights on your repo
< dongcarl>
sipa: many thanks!
< sipa>
ok, all done, i think
< cfields>
sipa: thanks!
< cfields>
I just commented on the lone migrated issue to make this all seem worthwhile.