< bitcoin-git>
[bitcoin] JeremyRubin opened pull request #15969: Refactor: explicit VerifyScript control flow based on pattern matching (master...explicit-verifyscript) https://github.com/bitcoin/bitcoin/pull/15969
< gmaxwell>
test/sanitizer_suppressions/ubsan contains suppressions of what appears to be actual undefined behavior, which I can't find any discussions of anywhere.
< jeremyrubin>
sipa: why does taproot need a new witness version if the WITNESS_V1_TAPROOT_SIZE != WITNESS_V0_SCRIPTHASH_SIZE?
< sipa>
jeremyrubin: because of a stupid mistake in bip141 that makes v0 segwit with program sizes other than 20 or 32 invalid
< jeremyrubin>
got it
< gmaxwell>
to be fair, I believe it was an intentional decision, jut a bad one. :P
< jeremyrubin>
Also this is kinda stupid, but the SIGTYPE::ECDSA/SIGTYPE::SCHNORR can be a template parameter because we never (in the code I've reviewed) use it without knowing it's type as a literal
< sipa>
jeremyrubin: sure, but premature optimization :)
< gmaxwell>
probably doesn't change the output code.
< sipa>
it likely does (there's some indirection)
< jeremyrubin>
Ah not so much as an optimization, but as better code (IMO) safety because someone never does something silly with dynamically figuring out what kind of signature you are passing in
< sipa>
it makes the code harder to review too
< jeremyrubin>
only a little -- I think it's easier because I spent some time looking to see how we were determining what type signature to pass in -- fortunately at present it's literals
< sipa>
but i agree it would make some classes of bugs harder
< jeremyrubin>
yeah it's not something that like *has* to change, just thought it was worth pointing out that the SignatureType argument looked a little smelly to me
< jeremyrubin>
compiler errors > assert(false)
< jeremyrubin>
sipa: BTW; could you open a Pull Request on your local repo
< jeremyrubin>
sipa: that way can add review comments/notes
< sipa>
yes, you can?
< jeremyrubin>
I mean github fork. I don't think I can open one on your fork
< sipa>
sure you can
< sipa>
there are a number of prs against my repo
< jeremyrubin>
can you update your master it's out of date
< jeremyrubin>
(figured out how to do it)
< sipa>
u
< sipa>
you shouldn't care about my master
< sipa>
also, this isn't really the time to go perfect the code yet
< jeremyrubin>
Well I'm opening the PR of your taproot against your master -- if I open it against bitcoin/master it will show up in core's PR?
< sipa>
you can choose both the base repo and branch
< jeremyrubin>
Yeah I just want to review to understand the trade offs better for looking at the specs
< gmaxwell>
sipa: you should blow it away, like my secp256k1 https://github.com/gmaxwell/secp256k1/ (incidentally yours has come back, IIRC you'd done the same bfore)
< jeremyrubin>
sipa: maybe I'm missing something but when I open a PR on your fork, it has to be against one of your branches.
< kallewoof>
sipa: stupid question, but where is the epoch stored? It is used in the SHA256 for the transaction digest, but is it just assumed to be 0 for now?
< sipa>
kallewoof: it is not stored. it's defined to be 0
< gmaxwell>
kallewoof: it isn't, it's implicit.
< jeremyrubin>
I have absolutely no clue how to do this.
< gmaxwell>
later a new checksig operation or something might be added that defines it to be 1
< sipa>
something somewhere sometime someplace may use the same hashing algorithm with another epoch value
< sipa>
it's likely never needed
< * jeremyrubin>
bangs head on keyboard
< kallewoof>
okay. so once we introduce epoch 1 and there may be ambiguity, we start storing it..?
< sipa>
no
< kallewoof>
gmaxwell: oh i see. that makes sense, thanks.
< sipa>
it's just part of the hashing algorithm
< kallewoof>
the purpose of the epoch is to avoid users being able to spend an output using a different operation that happens to use the same signature and "content" (content being the sighash sans the epoch)
< kallewoof>
IIUC
< gmaxwell>
right
< gmaxwell>
like in some v2 script it might be defined to be 1, which will prevent you from rebinding a signature from a v2 pubkey to v1... but in no case would it need to be signaled
< kallewoof>
got it!
< jeremyrubin>
is 1 byte kinda small?
< sipa>
1 byte is infinite
< sipa>
:)
< jeremyrubin>
we can extend it if we hit 255?
< sipa>
yes
< gmaxwell>
one bit would be sufficient, but would complicate alignment.
< sipa>
all we need is making sure no preimage ever collides
< sipa>
if the existing extension mechanisms are insufficients, we can always switch to a different tag in the hash or so
< jeremyrubin>
In the future how do we infer the epoch? Like from tx version or something?
< jeremyrubin>
I guess it doesn't matter that much...
< jeremyrubin>
Just trying to determine the benefit of doing this approach v.s. having a pre-salted SHA256
< sipa>
jeez peoe
< sipa>
if the byte wasn't there nobody would even talking about these issues
< sipa>
it's just a mechanism for future extension, like so many others
< wumpus>
gmaxwell: no, there was no discussion of specific surpressions as far as I know, this was for adding a travis run to catch new UBsan problems, so all the current ones had to be surpressed for that to not fail every single time :)
< sipa>
wumpus: not releasing right after new year is probably good
< wumpus>
sipa: so that's a vote for moving it, I guess?
< wumpus>
I'm just asking to be sure that having two months less than initially expected doesn't ruin anyone's plans
< wumpus>
"not doing a release on new year" is a good point, although it's quite a nice release date :-)
< gmaxwell>
"We illustrate ur approach" written by lolcats?
< jonasschnelli>
heh
< wumpus>
interesting approach
< jeremyrubin>
sipa: the annex is a bit unclear to me -- my understanding is that it's a space to provide an extra data argument? But what stops this from being malleated? It's covered by the signature? Is one annex certainly enough? What if I want 2
< jeremyrubin>
Sorry if those are bad questions
< wumpus>
at least it avoids the 'assebmly language is hard to review' problem by the code being provable correct at every manipulation step
< jeremyrubin>
I think what I would expect is that when I open a taproot i climb back through the witness data up to the 0xc0 leaf
< gmaxwell>
jeremyrubin: the annex is signed.
< jeremyrubin>
Ok -- I think I'd rather the spec say 'covered by the wtxid' rather than 'transaction digest' (less ambiguous by a smidge)
< jeremyrubin>
If a particular tap branch doesn'
< jeremyrubin>
* doesn't require a signature, then the annex is malleable (doesn't affect txid so no big deal I guess)
< gmaxwell>
if an output can be spent without a signature it's going to be malleable in many ways, including txid impacting ones. :)
< jeremyrubin>
That's kind of true -- could imagine a world where it's not true
< gmaxwell>
regardless, the annex is protected from malleation in the same way anything else is (and more so than many things are)
< jeremyrubin>
Fair -- might be good to introduce a OP_NO_ANNEX or something at some point if annexes are doing anything... interesting.
< jeremyrubin>
is there anywhere the annex has been discussed more in detail on what it might be used for?
< gmaxwell>
the main purpose of the annex is to be able to adjust transaction weight prior to looking up inputs.
< gmaxwell>
(well to be clear, its a generic signed data extension... but the construction is specifically motivated so it can work inputlessly)
< jeremyrubin>
Can you calrify that a little bit for me? {inputlessly, adjust transaction weight}
< gmaxwell>
opcodes which are very expensive to verify would increase the transaction's weight, but its important that we can check the effective weight of a transaction before doing the work of verifying it (in particular, looking up the inputs).
< jeremyrubin>
OK, so the idea of an annex is to specify a non malleable 'promise' of how much work validation will cost
< gmaxwell>
right, or rather it's an extension mechenism which is sufficient for that application. though it might turn out to have other ones later.
< jeremyrubin>
and if we ever see a txn which breaks that contract we ban the peer who sent or something
< gmaxwell>
Right.
< jeremyrubin>
gotcha -- annex is a peculiar word
< jeremyrubin>
hint/heuristic would pass the intention to me much better
< gmaxwell>
It's just a general extension mechenism.
< jeremyrubin>
But if we don't yet know what it might be for, then something more general is fine
< gmaxwell>
The only real relationship to the application is that application requires the extension payload be in the transaction, that it be unambigiously interpertable without knowing the input script, and that it get signed.
< gmaxwell>
(also: because of the final fork, we couldn't add the annex later so easily...)
< gmaxwell>
(by final fork, I mean that it has to be signed)
< gmaxwell>
otherwise we would have left it out.
< jeremyrubin>
Ok, so new question: why *not* pass the annex to the stack?
< jeremyrubin>
just less efficient?
< jeremyrubin>
But seems crappy if there's a use case where you'd want to pass it
< gmaxwell>
if you want data on the stack, just put data onto the stack.
< jeremyrubin>
I guess we can just introduce an opcode THIS_DATA_WAS_THE_ANNEX and include it twice
< jeremyrubin>
gmaxwell: you might both want to pass the data and know that it was the annex
< jeremyrubin>
or I guess FETCH_ANNEX also works
< gmaxwell>
right, though I can't imagine what this would be useful for, but sure that could be done.
< gmaxwell>
more likely would be a particular opcode that doesn't access the annex data directly but checks an annex controlled property.
< jeremyrubin>
Oh I do! We could enforce that if a txn is signed with an innacurate annex-as-a-validation-cost-hint it becomes anyone can spend
< jeremyrubin>
Yeah exactly
< gmaxwell>
like CSV doesn't directly muck about bits of the transactions serialization, it just imposes constraints on the locktime.
< jeremyrubin>
Some sort of use case around being able to ensure that if a branch is taken, the proper annex is set
< jeremyrubin>
(either with anyonecan spend teeth or just invalid is fine too ;) )
< gmaxwell>
that sounds like a great way to steal a hardware wallet's coins...
< gmaxwell>
the idea behind annex based costs is that they'd be required to be accurate (or at least greater) than the actual cost.
< gmaxwell>
(and you could drop peers that give you junk)
< jeremyrubin>
I guess one issue with an annex like thing is that if it's being used to do a p2sh like thing in the future as a hacky graftroot delegation scheme, we really can't pass data to it
< gmaxwell>
there shouldn't be any reason to do that.
< jeremyrubin>
I guess to make my point more succinctly, the encoding scheme for detecting tapscript spends and annexes feels kinda hacky
< jeremyrubin>
We're limited by the 520 byte limit, but I wonder if we'd be better off encoding all this info into a serialized struct rather than something that has witness program semantics
< jeremyrubin>
Anyways, headed to sleep. will continue pondering
< bitcoin-git>
[bitcoin] practicalswift opened pull request #15971: validation: Add compile-time checking for negative locking requirements in LimitValidationInterfaceQueue (master...negative-locks) https://github.com/bitcoin/bitcoin/pull/15971
< promag>
MarcoFalke: do you mind reviewing #14984?
< bitcoin-git>
[bitcoin] promag opened pull request #15975: univalue: Drop overloaded members (master...2019-05-drop-univalue-overloads) https://github.com/bitcoin/bitcoin/pull/15975
< bitcoin-git>
[bitcoin] joemphilips closed pull request #11770: [REST] add a rest endpoint for estimatesmartfee, docs, and test (master...rest_fee) https://github.com/bitcoin/bitcoin/pull/11770
< bitcoin-git>
[bitcoin] instagibbs closed pull request #13045: [p2p] getblock for 1-block reorgs in response to compact block message (master...cmpcttie) https://github.com/bitcoin/bitcoin/pull/13045
< bitcoin-git>
[bitcoin] MarcoFalke closed pull request #15045: [test] Apply maximal flags to tx_valid tests and minimal flags to tx_invalid tests (master...min_txtests_flags) https://github.com/bitcoin/bitcoin/pull/15045
< bitcoin-git>
[bitcoin] MarcoFalke reopened pull request #15045: [test] Apply maximal flags to tx_valid tests and minimal flags to tx_invalid tests (master...min_txtests_flags) https://github.com/bitcoin/bitcoin/pull/15045
< bitcoin-git>
[bitcoin] practicalswift closed pull request #15721: validation: Check absence of locks at compile-time (LOCKS_EXCLUDED) in addition to the current run-time checking (AssertLockNotHeld) (master...negative-locking-annotations) https://github.com/bitcoin/bitcoin/pull/15721
< bitcoin-git>
[bitcoin] DrahtBot closed pull request #14049: Enable libsecp256k1 ecdh module, add ECDH function to CKey (master...2018/08/bip151_ecdh) https://github.com/bitcoin/bitcoin/pull/14049
< bitcoin-git>
[bitcoin] DrahtBot reopened pull request #14049: Enable libsecp256k1 ecdh module, add ECDH function to CKey (master...2018/08/bip151_ecdh) https://github.com/bitcoin/bitcoin/pull/14049
< bitcoin-git>
[bitcoin] DrahtBot closed pull request #14033: p2p: Drop CADDR_TIME_VERSION checks now that MIN_PEER_PROTO_VERSION is greater (master...nVersion-checks) https://github.com/bitcoin/bitcoin/pull/14033
< bitcoin-git>
[bitcoin] DrahtBot reopened pull request #14033: p2p: Drop CADDR_TIME_VERSION checks now that MIN_PEER_PROTO_VERSION is greater (master...nVersion-checks) https://github.com/bitcoin/bitcoin/pull/14033
< bitcoin-git>
[bitcoin] DrahtBot closed pull request #13818: More intuitive GUI settings behavior when -proxy is set (master...2018/07/gui-proxy) https://github.com/bitcoin/bitcoin/pull/13818
< bitcoin-git>
[bitcoin] DrahtBot reopened pull request #13818: More intuitive GUI settings behavior when -proxy is set (master...2018/07/gui-proxy) https://github.com/bitcoin/bitcoin/pull/13818
< bitcoin-git>
[bitcoin] DrahtBot closed pull request #13360: [Policy] Reject SIGHASH_SINGLE with output out of bound (master...insecure_single) https://github.com/bitcoin/bitcoin/pull/13360
< bitcoin-git>
[bitcoin] DrahtBot reopened pull request #13360: [Policy] Reject SIGHASH_SINGLE with output out of bound (master...insecure_single) https://github.com/bitcoin/bitcoin/pull/13360
< bitcoin-git>
[bitcoin] DrahtBot closed pull request #12578: gui: Add transaction record type Fee (master...2018-03-fee-transaction-record) https://github.com/bitcoin/bitcoin/pull/12578
< bitcoin-git>
[bitcoin] DrahtBot reopened pull request #12578: gui: Add transaction record type Fee (master...2018-03-fee-transaction-record) https://github.com/bitcoin/bitcoin/pull/12578
< wumpus>
wow what's drahtbot doing
< gwillen>
I have been told that it periodically closes and reopens old PRs to get travis to re-run
< gwillen>
although I feel like I don't remember seeing it do this before recently
< gwillen>
perhaps the git integration could be taught to suppress those
< wumpus>
it does, it's just that it goes kind of wild now
< wumpus>
hmm maybe, some kind of debouncing, if a PR is reopened within N seconds of it being closed, ignore the event
< gwillen>
that would work, it looks like it takes about 20 seconds
< gwillen>
if you're already keeping state for that, you could also buffer events until it's been ~60 or 90 seconds without one, before you connect to IRC and send them all
< gwillen>
to reduce the joins and parts
< wumpus>
right
< bitcoin-git>
[bitcoin] jamesob opened pull request #15976: refactor: move methods under CChainState (pt. 1) (master...2019-05-au-cchainstate) https://github.com/bitcoin/bitcoin/pull/15976
< provoostenator>
(I shadowbanned myself again, really need to fix my bouncer setup)
< sipa>
?
< provoostenator>
Basically my IRC client shows my messages in the channel, but they don't actually show up.
< provoostenator>
Until I do an /identify
< provoostenator>
Whereas DMs work fine.
< provoostenator>
This started happening on and off a weeks ago, so it's probably a useful security feature, just lack of error handling in client (Textual)
< sipa>
sounds like you need a better irc client
< provoostenator>
Indeed
< provoostenator>
sipa: can you make a pull request to yourself with the "tapoot" brach, or a Draft pull request to Bitcoin Core? I find that easier to review than just a branch.
< provoostenator>
PR description is also a nice place for "try these steps to test this"
< sipa>
provoostenator: no, i think it's far too early for that
< provoostenator>
Isn't that what the Draft feature is for?
< sipa>
the code is there for demonstration, to see what kind of complexity is involved, but review at this point should be on the bip
< provoostenator>
Right, it's tempting to review in too much detail, that makes sense.
< gmaxwell>
the drahbot thing makes ordering prs by activity suck, btw.
< luke-jr>
:<
< luke-jr>
why can't it just trigger Travis restarts directly?
< gmaxwell>
jamesob: thansk for benchmarking that PR. It'll be interesting if its not actually faster (as that woudl imply copying a hasher is slower than creating one plus one compression run ... which if its true suggests something else needs to be fixed). This is why we test, of course.
< achow101>
luke-jr: how would it?
< achow101>
i don't think you can have travis start a build on a pr as if a new commit was pushed. iirc the point of closing and reopening triggers travis to do a new build which means it will use the merge commit with the current master which is what we want
< jamesob>
gmaxwell: sure thing. Left another bench running against the rebase, but afk. Will report back