< sipa> achow101: i'm breaking my head... i don't understand how ProduceSignature ever worked at all
< achow101> sipa: where?
< sipa> if you call it through SignSignature, from multiple SPKMs, as far as i can see, if the first SPKM is able to sign, the second one will just wipe the witness
< achow101> It'll do VerifyScript and find it to be valid
< sipa> oh
< achow101> Then abort early and not change anything
< sipa> got it!
< sipa> this explains
< sipa> thanks
< sipa> it constructs a new checker objects, which doesn't have access to PrecomputedTransactionData, which is necessary to verify taproot sigs
< achow101> Yeah
< achow101> I have a pr for that actually
< sipa> oh?
< achow101> (for other reasons, but should give access to that)
< sipa> passing down the signaturecreator's checker, rather than constructing a new one?
< achow101> #21166
< gribble> https://github.com/bitcoin/bitcoin/issues/21166 | Have SignatureExtractorChecker subclass MutableTransactionSignatureChecker by achow101 · Pull Request #21166 · bitcoin/bitcoin · GitHub
< sipa> no, that doesn't work
< sipa> it's still creating a new checker
< sipa> (which includes a new MutableTransactionSignatureChecker)
< achow101> Ah, think I just don't know what you need
< sipa> yeah, it's a fine change, but it's orthogonal
< sipa> also: really, we're verifying every existing witness in a transaction being signed in every SPKM?
< achow101> I guess we are
< sipa> i'm sure there's a way to keep it in SignatureData form across those calls
< sipa> which would avoid that
< sipa> also orthogonal, of course
< achow101> Yeah. Signing performance hasn't really been a concern too
< sipa> yeah
< sipa> just seems a bit silly, it's not a big deal
< sipa> achow101: can you explain how 21166 fixes 21151? what function specifically is missing?
< achow101> CheckSequnce was missing
< sipa> i see
< achow101> Same with ChecLockTime, so op_cltv scripts don't work too
< sipa> yeah
< sipa> achow101: i think there is a simpler solution... just don't wipe scriptWitness
< sipa> unless sigdata.witness is false
< achow101> what about scriptSig?
< sipa> there are a couple things
< sipa> ideally just these functions don't get called at all for things that are already signed
< sipa> the caller has the best context to determine that, i think
< sipa> caller = wallet, rpc, psbt, ...
< achow101> not necessarily
< sipa> e.g. if you lack amount information, a VerifyScript will fail for every witness
< sipa> that doesn't mean you should wipe it
< achow101> for these non-wallet inputs, no context knows until it runs through the verifier
< sipa> ok, setting aside the question of deciding whether an input is fully signed or not
< sipa> if you somehow think it's not yet fully signed, so try to sign yourself, and fail... there is no point in wiping anything at all
< achow101> right
< sipa> and i think that's a pretty simple change
< achow101> that might be a simple change, but I think a better/longer term solution is to overhaul the signing to be more psbt like
< achow101> so it puts things inside of SignatureData and only attempts to create a scriptSig and scriptWitness when trying to finalize
< sipa> agree
< achow101> and those shouldn't be part of the SignatureData
< sipa> part of the complexity is the compatibility with the old raw transaction behavior where we want to be able to go from tx -> sigdata
< sipa> but we don't need that functionality for new script signing functionality
< achow101> I really just want to deprecate and remove signrawtransaction*
< achow101> but I think people still use it
< sipa> but we probably also want a function that can detect invalid scriptSig/witness data, so it can ignore it
< sipa> otherwise it'd just be: you already have witness/scriptSig -> no touch
< sipa> made using sendtoaddress :)
< achow101> sipa: miniscript?
< achow101> or taproot?
< sipa> taproit
< sipa> *taproot
< achow101> nice
< sipa> branch needs a lot of cleaning up, but it does something now :)
< bitcoin-git> [bitcoin] MarcoFalke pushed 3 commits to master: https://github.com/bitcoin/bitcoin/compare/5ba5becbb5d8...fb67caebe26f
< bitcoin-git> bitcoin/master ace3f4c Jon Atack: test: improve assertions in feature_blockfilterindex_prune.py
< bitcoin-git> bitcoin/master 88c4b9b Jon Atack: test: remove unneeded node from feature_blockfilterindex_prune.py
< bitcoin-git> bitcoin/master fb67cae MarcoFalke: Merge #21297: test: feature_blockfilterindex_prune.py improvements
< bitcoin-git> [bitcoin] MarcoFalke merged pull request #21297: test: feature_blockfilterindex_prune.py improvements (master...feature_blockfilterindex_prune-improvements) https://github.com/bitcoin/bitcoin/pull/21297
< bitcoin-git> [gui] hebasto opened pull request #229: Fix regression with initial sorting after pr205 (master...210227-sort) https://github.com/bitcoin-core/gui/pull/229
< bitcoin-git> [gui] luke-jr opened pull request #230: Support backup to new text-based database dump format (master...gui_backup_formats) https://github.com/bitcoin-core/gui/pull/230
< jonatack> vasild: I finished reviewing the I2P PR late last night, then my internet went down, and when I restarted GitHub had saved none of the many comments I had pending. Disheartening. I'm thinking that by default now on large PR reviews I'll start annotating the code directly, git commit and push my own branch, then refer to the branch in my review. That also takes much less GitHub space,
< jonatack> which is good in order to avoid deeper "hidden items" clicking, which for me is up to 105 hidden items in that PR, and which continued to hide one of your commits even after clicking repeatedly to unhide everything.
< jonatack> (Not a reflection on the PR itself--it looks very good!)
< bitcoin-git> [bitcoin] ivanacostarubio opened pull request #21308: build: removing xcrun from darwin build (master...issue18959) https://github.com/bitcoin/bitcoin/pull/21308
< bitcoin-git> [bitcoin] theStack opened pull request #21310: zmq test: fix sync-up by matching notification to generated block (master...2021-zmq-fix_sync_up_procedure) https://github.com/bitcoin/bitcoin/pull/21310
< bitcoin-git> [gui] hebasto closed pull request #228: Drop unused parameter in GUIUtil::get{Save|Open}FileName (master...210224-file) https://github.com/bitcoin-core/gui/pull/228
< luke-jr> is interface_zmq having issues, or is there some incomprehensible bug in gui#230 I can't reproduce locally? :/
< gribble> https://github.com/bitcoin/bitcoin/issues/230 | Update German translation to latest git. by TheBlueMatt · Pull Request #230 · bitcoin/bitcoin · GitHub
< * luke-jr> peers at gribble
< jeremyrubin> if I make a PR, and then I make another PR referincing that PR in the title, and then I change the first to reference the latter, and then put the number in IRC what does gribble do?
< luke-jr> I doubt gribble responds to itself
< luke-jr> gribble: echo gribble: echo hi
< luke-jr> x.x
< luke-jr> jeremyrubin: also, you'd need to be quick - gribble doesn't announce renames ;)
< luke-jr> err, maybe not relevant if you manually enter it
< luke-jr> anyway, off topic XD
< phantomcircuit> jeremyrubin, it ignores itself as all irc bots that aren't instantly banned do
< bitcoin-git> [bitcoin] theStack opened pull request #21311: rpc: document optional fields for getchaintxstats result (master...2021-rpc-document_optional_getchaintxstats_fields) https://github.com/bitcoin/bitcoin/pull/21311
< bitcoin-git> [bitcoin] vladyslavstartsev opened pull request #21312: wallet: remove lock during `listaddressgroupings` (master...listaddressgroupings-no-lock) https://github.com/bitcoin/bitcoin/pull/21312
< s7r> building from source doesn't work any more cleanly on Debian.
< s7r> I get A compiler with support for C++17 language features is required.
< s7r> (current compiler does support C++17 though)
< sipa> which compiler is that?
< s7r> sipa, gcc
< sipa> which version?
< s7r> 4:6.3.0-4
< s7r> sorry: 6.3.0-18+deb9u1
< sipa> that may be too old