< gmaxwell> luke-jr: so whats the deal with this sigops checking thing. Help me understand your position.
< luke-jr> gmaxwell: does the comment I just posted help? or is there still confusion on it?
< luke-jr> (literally seconds ago)
< * gmaxwell> goes to read
< GitHub99> [bitcoin] pstratem opened pull request #8376: Fix exception message to reference actual thrower. (master...2016-07-19-cwallet-sethmasterkey) https://github.com/bitcoin/bitcoin/pull/8376
< gmaxwell> okay, I see. We probably should make bare multisig non-standard, but it would be inadvisable to do that without a lot of notice and time.
< gmaxwell> It shouldn't be done accidentally on the sly via another fix.
< luke-jr> sure, I consider bare multisig policy entirely unrelated to these 2 PRs
< CodeShark> what's bare multisig?
< luke-jr> I think both PRs should be merged (once sipa's is updated to not remove the current bytespersigop)
< luke-jr> CodeShark: multisig without p2sh
< sipa> luke-jr: my PR does not remove -bytespersigop
< luke-jr> CodeShark: accidentally broken in 0.12, fixed in #8364
< luke-jr> sipa: it removes the current bytespersigop and adds a new one that does soemthing different.
< gmaxwell> luke-jr: okay, so if we're ordering transactions by the resources they actually use, and we'll make bare multisig non-standard via another path-- then why do we need bytespersigop?
< luke-jr> gmaxwell: for its original purpose: to filter spam misusing multisig
< gmaxwell> well what kind of spam? people trying to store lots of data don't have a problem having more bytes.
< gmaxwell> If someone is trying to burn signature operations, we're protected by the sigops limit and protected by the fact that sigops limit burning transactions have to pay as much fees as a set of ordinary txn that burns as many sigops.
< luke-jr> the "make tx take long to check" kind that has an old CVE I can't lookup because the wiki is apparently down(!) :/
< luke-jr> nm, finally loaded: CVE-2013-2292
< luke-jr> and apparently there's some kind of spam that dex guy wants to abuse it for as well
< gmaxwell> yes, but the dex guy won't get hit by the current limit, he may be too ignorant to realize this.
< luke-jr> simply saying "pay a higher fee" sends the message that the spam is encouraged; at least by padding people can't claim that
< gmaxwell> luke-jr: so the n minutes to verify thing is stopped by the maximum standard transaction size being 100k. It's a sighash gringing bug. You actually don't want small transactions to trigger that.
< gmaxwell> luke-jr: in particular they pay as much (or likely more) fee than doing the attack with more ordinary looking transactions.
< gmaxwell> So the abusive use really isn't help, abusers will adapt to do whatever works best for them.
< luke-jr> gmaxwell: how do we avoid people looking at this change and saying "see? as long as I pay the higher fee, I have a right to spam" ?
< gmaxwell> and we are currently left with bare multisig users that, we know know, have problems reliably producing transactions under this policy.
< luke-jr> gmaxwell: #8364 entirely fixes the current problem
< gmaxwell> luke-jr: by stating it outright that it's not intended to enable transactions that store unrelated data in the blockchain or intentionally waste resources.
< gmaxwell> E.g. we could release note that.
< luke-jr> like we did with OP_RETURN?
< luke-jr> we all know how that turned out..
< gmaxwell> luke-jr: but it adds a lot of code that is non-obvious in its exeuction and application.. and which transacting parties should have to model themselves to know if they pass the test or don't.
< luke-jr> huh? #8364 would fix the bug so transacting parties are never affected
< luke-jr> (sipa's change would probably do that, though)
< gmaxwell> Yea, I think sipa's change is necessary and sufficient. and 8364 might be okay, but it's more complexity with potential surprise consequences, which at best I wouldn't want to rush out with.
< gmaxwell> and I think it's not actually needed, except in terms of making it clear that we don't support people storing arbritary non-transaction data in the blockchain.
< luke-jr> what potential surprise consequences? it's a simple bugfix to existing released code, that only allows more transactions that were false-positives
< luke-jr> re needed or not: keep in mind miners set their own policies, and the defaults of (eg) disabling bare multisig can be overridden
< luke-jr> miners might want to continue mining bare multisig after defaults change, yet not open this attack risk
< sipa> we're really talking about different problems
< luke-jr> if de facto we end up in a situation where nobody is mining multisig, I guess that'd be a good time to remove the current bytespersigop.. but that's 0.14 at the earliest unless miners change it manually AIUI
< sipa> you're concerned about sending an unintentional signal that high-sigop transactions are ok
< sipa> i'm concerned about blocks being filled with garbage
< gmaxwell> s/are okay/are okay if you pay some market fee/
< sipa> i think people will use whatever means necessary to store data in the chain, and saying that this must be stopped for the system to succeed is being blind for reality
< sipa> however, we do now actually have a fee market
< gmaxwell> I can write some release note text that says that the network capacity is a shared resource and that just because a usecase is technically possible it doesn't mean that it's an intentionally supported usecase.
< luke-jr> sipa: if reality is they will, then reality is highly against Bitcoin surviving
< CodeShark> sipa: I agree - we should not be deciding use cases, only working to ensure incentives don't get out of whack
< sipa> luke-jr: and policy (and even consensus rules) can be proposed to combat certain behaviour that damages shared resources
< luke-jr> miners are less likely to mine spam-padded-to-bypass-spam-limits than spam-just-paying-a-higher-fee IMO
< luke-jr> intentionally*
< gmaxwell> luke-jr: yea, slightly,-- which is the argument against ltc like 'dust' limits. But it's a minor detail at this point.
< sipa> but 8364 on itself would allow high-consensus-sigop low-accurate-sigops transactions into blocks, allowing anyone on the network to fill a block with garbage for 1/20th of the market price
< gmaxwell> They're more likely to do harmful self-profiting things rather than long term community benefiting things if the defaults are so out of wack that they get encouraged by people to change them.
< sipa> that is a directly exploitable bug we'd be shipping in our mining code
< sipa> and much more serious than a worry about a potentially misinterpreted signal _to people who are already using it anyway_
< luke-jr> what justification is there to exclude/replace the current bytespersigop this close to release? it does no harm at worst (especially if sipa's policy change is also included)
< gmaxwell> not just exploitable, previously actively exploited.
< luke-jr> sipa: I'm arguing for *both*, not 8364 *instead of* yours
< sipa> luke-jr: ok, fair enough
< gmaxwell> luke-jr: the _current_ unmodified bytes per sigops blocks some bare multisig usage.
< luke-jr> gmaxwell: accidentally
< gmaxwell> Sure. It's a bug, we need to fix it.
< luke-jr> so we fix that, and independently add sipa's policy.
< sipa> luke-jr: both together are acceptable, but more complexity, and i'm not sure what it gains
< gmaxwell> So to fix that we have to do something there. One option is to take it out. The other option is to do 8364. I think take it out is superior because with 8365 the marginal benefit from 8364 is small relative to the code complexity-- esp-- as you note, this close to release.
< * luke-jr> notes 8364 has been suggested as a fix without any controversy for months now.
< sipa> but you agree that 8364 (on its own) would be a bad idea?
< luke-jr> sipa: I don't believe I have sufficient information to answer that.
< luke-jr> or rather, I have to do some analysis of the information I have, that i have not yet done
< sipa> as far as i'm concerned, the only thing to prevent is having the mining code get confused by transactions, and producing a block that nobody would want to create
< gmaxwell> luke-jr: without 8365, 8364 would reintroduct the consensus-sigops block exhaustion attack that was being actively exploited.
< sipa> wrt concern about resource usage, we should propose to make bare multisig nonstandard
< gmaxwell> we should. But I think thats is seperate, perhaps just as a note in the release notes.
< luke-jr> seems like more of an economic concern than technical? after all, people could just pay 20x higher fees. admittedly, not a good situation of course
< luke-jr> seems just doing 1-of-2 baremultisigs legitimately might actually be an economic concern in the same sense
< * luke-jr> wonders why he has to defend "not 8365's new policy" when he has never argued for omitting it :p
< gmaxwell> hah.
< gmaxwell> My comment was just a reply to "notes 8364 has been suggested as a fix without any controversy for months now" -- apparently I must have missed that fix discussion since I would have complained that it reopened the attack. :)
< luke-jr> IIRC
< luke-jr> maybe it got lost in the other bs on that thread
< sipa> i thought 8365 was the obvious fix all the time :)
< luke-jr> bbiab
< sipa> (because i assumed #7081 was intended to fix the sigop exhaustion attack, and its effect on relay was just a side effect instead of an intentional policy)
< gmaxwell> I missed 8079's discussion (because it got kind of hostile at some point).
< GitHub106> [bitcoin] pstratem opened pull request #8377: Rename usdhd option to createhdwallet (master...2016-07-16-cwallet-createhdwallet) https://github.com/bitcoin/bitcoin/pull/8377
< phantomcircuit> jonasschnelli, fyi qa/rpc-tests/wallet-hd.py passes even when i changed usehd to createhdwallet
< phantomcircuit> which means it's broken maybe?
< phantomcircuit> CHDChain & CKeyMetadata in walletdb.h seem like they could be in wallet.h instead
< phantomcircuit> they're not really specific to the walletdb
< phantomcircuit> otoh maybe it's reasonable that the abstraction be improved such that they are?
< michagogo> Okay, PR created -- looks like it didn't happen automatically because GitHub moved their API endpoint, and my Octokit was outdated.
< phantomcircuit> sipa: apparently returning a CKey doesn't work?
< GitHub9> [bitcoin] pstratem opened pull request #8378: Move SetMinVersion for FEATURE_HD to SetHDMasterKey (master...2016-07-19-cwallet-initloadwallet-ordering) https://github.com/bitcoin/bitcoin/pull/8378
< GitHub21> [bitcoin] laanwj pushed 2 new commits to master: https://github.com/bitcoin/bitcoin/compare/045106b4f13c...c98abf2c7099
< GitHub21> bitcoin/master 3b3ce25 Cory Fields: build: fix non-deterministic biplist...
< GitHub21> bitcoin/master c98abf2 Wladimir J. van der Laan: Merge #8373: Fix OSX non-deterministic dmg...
< GitHub99> [bitcoin] laanwj closed pull request #8373: Fix OSX non-deterministic dmg (master...biplist-determinism) https://github.com/bitcoin/bitcoin/pull/8373
< GitHub168> [bitcoin] laanwj pushed 1 new commit to 0.13: https://github.com/bitcoin/bitcoin/commit/1fe7f404078121ad370ec955aa23befa322549bb
< GitHub168> bitcoin/0.13 1fe7f40 Cory Fields: build: fix non-deterministic biplist...
< wumpus> BlueMatt: the only thing that would have blocked rc1 yesterday is a critical issue that causes everything to catch fire - if we keep regarding last-minute non-critical changes as blockers, we can never get anything released
< wumpus> I agree it should be solved obviously
< phantomcircuit> wumpus, updated the comment in #8378 think it needs to be tagged for 0.13
< wumpus> tagged
< Raccoon> +--------------+ +--------+ +-----+ +--------+ +------+ +------+ +---+
< Raccoon> | #BITCOIN-DEV |## | MISSES |-  | YOU |\\ | GAVIN, |## | COME |\\ | HOME |-  | ! |::
< Raccoon> +--------------+## +--------+ | +-----+\\ +--------+## +------+\\ +------+ | +---+::
< Raccoon> ################ '--------  \\\\\\\ ########## \\\\\\\\ '------  :::::
< luke-jr> !ops
<@luke-jr> eh, he's not here? O.o
< phantomcircuit> luke-jr, lol it's a notice and the modes are set wrong
< luke-jr> phantomcircuit: modes are set "wrong" because that's how GitHub does commits :x
< phantomcircuit> luke-jr, lol
< phantomcircuit> i think you can still set a ban to keep him from doing it
< luke-jr> hm
< jl2012> the signed osx binary seems not deterministic?
< wumpus> jl2012: no, the ds_store is not deterministic
< wumpus> (which is not part of the signed data, but still affects the hash)
< btcdrak> phantomcircuit: no you can set github to come to the channel to post commit notices
< jl2012> wumpus: so I should now rebuild for osx?
< wumpus> no, we'll just let this slide for rc1, for rc2 it will be solved
< jl2012> ok
< wumpus> (it's a change in the build system, not in the descriptor, so you can't easily build rc1 with that change)
< kinlo> hmmmz
< kinlo> this channel needs to be set +n (no messages from people not on the channel)
< kinlo> just make the github clients join the channel
< wumpus> then github can't paste into it
< btcdrak> wumpus: yes it can
< kinlo> github can join...
< btcdrak> it's a setting
< luke-jr> kinlo: way too spammy
< wumpus> I know, we had that in the beginning, but then github bots will be joining and parting all the time
< kinlo> luke-jr: and raccoon isn't ?:)
< luke-jr> Raccoon can be banned if he does it again
< luke-jr> he PM'd me he wouldn't
< wumpus> they distribute it over multiple bots, let's just ban the idiot and move on
< jonasschnelli> [23:45:16] <phantomcircuit:#bitcoin-core-dev> jonasschnelli, why does CHDKeyStore::PrivateKeyDerivation take the keypath as a string?
< jonasschnelli> phantomcircuit: I guess you are refering to one of my closed PR Bip32 PRs?
< jonasschnelli> I think somewhen we need a feature that can derive keys from a given string keypath.
< jonasschnelli> <*highlight>[04:46:15] <phantomcircuit:#bitcoin-core-dev> jonasschnelli, fyi qa/rpc-tests/wallet-hd.py passes even when i changed usehd to createhdwallet
< jonasschnelli> phantomcircuit: usehd=1 is default....
< jl2012> wumpus: v0 witness program scriptPubKey becomes standard before activation even in mainnet. Do we need to mention this in release notes?
< jl2012> Now it says "Testnet-only segregated witness" but it affects some mainnet policy
< wumpus> jl2012: does it affect users in any way, in practice?
< wumpus> I mean does it result in something useful that can be done already? if not I don't think it needs to be mentioned
< wumpus> it'll just confuse
< jl2012> only if someone generates bare segwit outputs. 0.13 will relay and mine these transactions
< wumpus> I don't understand, why is that the case for mainnet?
< sipa> spending them is non-standard, and won't be relayed before activation
< jl2012> because witness outputs are now starndard
< jl2012> I'm not talking about spending. I'm talking about sending to segwit outputs
< jl2012> precisely, if a scriptPubKey is OP_0 <20/32 bytes>, it becomes standard in 0.13
< gmaxwell> He means paying to a segwit output which doesn't use p2sh.
< jl2012> yes
< gmaxwell> They can't be encoded as addresses currently, they can't be spent (well spending is nonstandard). but you could pay to them just like you could pay to a p2sh segwit address (even now)
< wumpus> so, all in all, it's still useless right now
< gmaxwell> I think perhaps we sould leave them non-standard in mainnet until there is an address encoding.
< gmaxwell> because it's useless and allowing useless things get dumb behavior enshrined.
< wumpus> yes - I guess they could be (mis)used for e.g. data storage?
< wumpus> like bare multisig
< gmaxwell> not really any worse than p2pkh, (well 32 bytes instead of 20); but right now _any_ use is either going to be a mistake or something stupid.
< jl2012> gmaxwell: I think even LN clients will use p2sh-segwit until there is a new address format?
< jl2012> technically speaking, people could use native segwit with BIP70
< sipa> right
< wumpus> I'm not sure standardness is meant as a way to prevent people from doing something stupid if they try very hard
< wumpus> I think just not mentioning this is enough to have no one care about it
< michagogo> There seems to be some unintentional markup going on here. https://usercontent.irccloud-cdn.com/file/1WImycPE/IMG_0002.PNG
< gmaxwell> jl2012: yea, but not on a network without any segwit support-- fair that I should back that until segwit is working.
< wumpus> michagogo: looks like a line starting with #
< michagogo> wumpus: yep, makes sense
< sipa> michagogo: yeah, the line need rewrapping
< michagogo> (That's the 0.13 release notes)
< gmaxwell> as an aside, in the future new segwit script types should remain non-standard until a non-trivial number of blocks after activation to avoid people getting themselves pilfered from in a reorg.
< sipa> should we do that even for segwit relay now?
< gmaxwell> the concern is that the feature activates and then right at that block some genius starts paying to it, then there is a 1 block reorg and their coins are stolen by the new block right before activation.
< gmaxwell> obviously "don't do that" but it's pretty surprising, and even some experts have thought that the 2016 block quiet period in the activation would prevent that concern.
< sipa> well it's a one-line change
< sipa> check bip9 status at tip.GetAncestor(144) instead of at tip
< sipa> if you want a day delay
< gmaxwell> well we're not even triggering relay of output creation on activation now.
< gmaxwell> (and we can't for p2sh, but I suppose there a potential attacker wouldn't usually know the preimage)
< GitHub102> [bitcoin] jl2012 opened pull request #8379: Remove duplicated name in release notes (0.13...patch-14) https://github.com/bitcoin/bitcoin/pull/8379
< GitHub109> [bitcoin] laanwj closed pull request #8379: Remove duplicated name in release notes (0.13...patch-14) https://github.com/bitcoin/bitcoin/pull/8379
< GitHub104> [bitcoin] laanwj pushed 2 new commits to 0.13: https://github.com/bitcoin/bitcoin/compare/1fe7f4040781...f0ff08d7841c
< GitHub104> bitcoin/0.13 48b9208 Johnson Lau: Remove duplicated name in release notes
< GitHub104> bitcoin/0.13 f0ff08d Wladimir J. van der Laan: Merge #8379: Remove duplicated name in release notes...
< GitHub113> [bitcoin] laanwj pushed 2 new commits to master: https://github.com/bitcoin/bitcoin/compare/c98abf2c7099...8e048f40cc25
< GitHub113> bitcoin/master 6523fca Patrick Strateman: Move SetMinVersion for FEATURE_HD to SetHDMasterKey
< GitHub113> bitcoin/master 8e048f4 Wladimir J. van der Laan: Merge #8378: [Wallet]Move SetMinVersion for FEATURE_HD to SetHDMasterKey...
< GitHub114> [bitcoin] laanwj closed pull request #8378: [Wallet]Move SetMinVersion for FEATURE_HD to SetHDMasterKey (master...2016-07-19-cwallet-initloadwallet-ordering) https://github.com/bitcoin/bitcoin/pull/8378
< GitHub11> [bitcoin] laanwj pushed 1 new commit to 0.13: https://github.com/bitcoin/bitcoin/commit/ebea65121e6c62f6b6acd79408a681b987126a0d
< GitHub11> bitcoin/0.13 ebea651 Patrick Strateman: Move SetMinVersion for FEATURE_HD to SetHDMasterKey...
< btcdrak> My Pine64 with 2GB just finished a full sync, I started it Jul 14 00:45 UTC, and it finished Jul 20 09:55 UTC
< jannes> On topic of typos in release notes: "propagation relay" should be "propagation delay", I think.
< MarcoFalke> jannes: Pull request welcome :)
< sipa> jannes: indeed!
< jonasschnelli> btcdrak: nice! What block storage did you use? SDCard or USBStick? Did you had a corruption(s) during sync?
< btcdrak> jonasschnelli: I used a Samsung EVO+ 128MB, no corruption issues.
< jonasschnelli> btcdrak: Okay. I guess the bigger the SDCard space the less corruptions you will see (even if you don't use the space).
< jonasschnelli> Did you used prunining?
< btcdrak> jonasschnelli: No, no pruning. Full node.
< jonasschnelli> Technically with pruning its still a full node. :)
< jonasschnelli> Good to know.. I need to try the same on my Pine.
< btcdrak> jonasschnelli: It's a fuller node :-p
< jonasschnelli> ;-)
< jonasschnelli> We should sell Pine64 together with a nice case and some webbase control center for 50$ as "your bank at home".
< GitHub70> [bitcoin] jafaber opened pull request #8380: fix typo: propagation relay -> delay (0.13...patch-1) https://github.com/bitcoin/bitcoin/pull/8380
< jannes> MarcoFalke: sipa: Created pull request. I know git, but first time I use github. Took me a while, sorry if it's duplicate by now :)
< sipa> jannes: looks good
< GitHub65> [bitcoin] jl2012 opened pull request #8381: Make witness v0 outputs non-standard (master...patch-15) https://github.com/bitcoin/bitcoin/pull/8381
< GitHub128> [bitcoin] jonasschnelli closed pull request #6481: [mining] allow other signal listeners to provide scripts for mining (master...2015/07/enhance_mining_flexibility) https://github.com/bitcoin/bitcoin/pull/6481
< Chris_Stewart_5> sipa: I'm looking closer at CBloomFilter, and does nHashFuncs/nTweak need to be prefaced with a compact size int as well? https://github.com/bitcoin/bitcoin/blob/f17032f703288d43a76cffe8fa89b87ade9e3074/src/bloom.h#L50
< Chris_Stewart_5> Can't an 'unsigned int' be larger than 4 bytes?
< sipa> Chris_Stewart_5: no, they're just integers
< sipa> Chris_Stewart_5: READWRITE(x) just invokes the serializer/deserializer for x
< sipa> vectors are serialized as compactsize(vector.size()), and then a concatenation of all the elements
< sipa> and no, the serialization code (so far) relies on unsigned int being 4 bytes
< Chris_Stewart_5> Hmm some how I got in my head that 'unsigned int' size isn't constant, looks like I was wrong
< sipa> the C standard does not fix its size
< sipa> but we don't support any architectures where it isn't 4 bytes
< Chris_Stewart_5> oh ok, thanks for the clarification
< GitHub104> [bitcoin] dooglus opened pull request #8382: Fix formatting error (0.13...patch-4) https://github.com/bitcoin/bitcoin/pull/8382
< bsm117532> When using `signrawtransaction` on a witness output and the relevant privkey isn't known to bitcoind, it gives an erroneous error message: "Witness program hash mismatch".
< sipa> bsm117532: is the witness script known?
< sipa> ah, or it's p2wpkh
< bsm117532> It's p2wpkh
< bsm117532> I bet I'm the only person using *raw* rpc calls with witness txns ;-)
< instagibbs> bsm117532, that error should only pop up if it fails VerifyScript?
< bsm117532> instagibbs: Is it that it fails to find a key, so fails to sign it...then fails VerifyScript at the end of the function?
< bsm117532> I'd think this wouldn't be a failure but should spit out the same transaction with "complete": false
< instagibbs> looks like the error for p2wpkh is just checking if the stack is size 2?
< instagibbs> if the hash it seems is size 20
< instagibbs> sees*
< instagibbs> and a few lines before it, for the 32 byte case
< bsm117532> instagibbs: Yes, that's it. The witness stack size would be zero (because it's unsigned).
< instagibbs> ok so it pushes nothing, and fails later
< instagibbs> we should just be checking the return value of ProduceSignature, right?
< bsm117532> SCRIPT_ERR_WITNESS_PROGRAM_WITNESS_EMPTY is a better error if stack is empty
< instagibbs> it's a special case for p2wpkh
< instagibbs> not a great error message I agree though
< * bsm117532> is still trying to grok the VerifyScript line
< bsm117532> This loop calls VerifyScript after each vin it processes. So with multiple inputs, it will always fail, even if it can sign the txn!
< bsm117532> (My example only has one input)
< bsm117532> What happens with non-segwit p2pkh's here? Won't they fail too?
< instagibbs> hmm? why would it fail with multiple valid inputs?
< bsm117532> Because it signs them one at a time.
< instagibbs> they will fail as well, the error here i think is just not helpful
< bsm117532> And calls verifyscript after each one
< instagibbs> that's how signing works
< bsm117532> So if I read this correctly, signrawtransaction would be incapable of signing something with multiple unsigned inputs?
< instagibbs> No, it signs anything it can
< instagibbs> then checks that script to see if it's satisfied
< instagibbs> in our case it's not, but it needs to spit out a better error
< bsm117532> The VerifyScript is inside the loop though...
< bsm117532> It should be outside.
< instagibbs> No, each input scriptSig needs to be checked individually
< bsm117532> Oh I see, `i` is passed.
< bsm117532> Does anyone have a good WIP to incorporate that change? I can fix it with my segwit wallet improvements if not.
< instagibbs> bsm117532, it is true that the 32 byte version has an error already for empty witness
< instagibbs> seems useful as a debugging message for both
< bsm117532> If I read this correctly, `signrawtransaction` cannot produce a transaction such that "complete": false. That behavior is fine, if a little confusing. I can just give the other error message (witness program empty) instead.
< sipa> sure, the result of signrawtransaction can be complete:false ?
< sipa> for example in case of multisig and you only have some of the keys
< bsm117532> I agree that's logically what it *should* do, but that VerifyScript called for each input fails if it's unsigned.
< bsm117532> I could be reading it wrong though...
< sipa> where did you see this error, btw?
< bsm117532> Running: bitcoin-cli signrawtransaction
< bsm117532> for a key that isn't in my wallet. It's a simple one input, one output txn, both p2wpkh
< sipa> hmm, that should never return an error
< sipa> it should just not sign
< bsm117532> It appears that TransactionSignatureChecker should be checking a witness signature, but STANDARD_SCRIPT_VERIFY_FLAGS contains SCRIPT_VERIFY_WITNESS which causes VerifyScript to fail if the witness is absent.
< bsm117532> Could we remove SCRIPT_VERIFY_WITNESS from this call? https://github.com/bitcoin/bitcoin/blob/master/src/rpc/rawtransaction.cpp#L821
< sipa> i don't understand why we call that in the first place
< sipa> the only correctness check should be at the end, to determine the value of complete
< bsm117532> sipa: I agree
< instagibbs> "value of complete"?
< bsm117532> instagibbs pointed out that it's a per-input check...
< instagibbs> bsm117532, did you get back a partially signed txn?
< bsm117532> instagibbs: no, it fails. I'll pastebin it for you
< instagibbs> please do thanks
< bsm117532> https://www.zerobin.net/?c6f94fa664089370#rWaua+D/pcl0zvYh8DNwbjfh+XitXfJSNHEOHLscb+A=
< instagibbs> "hex"
< instagibbs> that's the partially signed
< bsm117532> Well it's not signed -- only one input.
< instagibbs> errr, yes
< instagibbs> but that's expected behavior if you don't have the key
< instagibbs> (potentially partially signed)
< bsm117532> Ok, like I said above, that's a reasonable behavior if it can't sign at all, in which case we can just change the error message.
< instagibbs> kk, agreed
< bsm117532> Now how to get it to say "I don't have the keys..." ;-)
< instagibbs> I think just add the 0 size check like the p2wsh has.
< bsm117532> Yes, I did that. Will include it in my wallet updates. Thanks!
< instagibbs> schweet
< instagibbs> do we want to augment TxInErrorToJSON to include witness data as hex?
< instagibbs> for a p2w{pkh/sh} we'll be given a blank scriptSig and nothing else
< bsm117532> Maybe...the error message is still unclear as to why it failed.
< bsm117532> BTW one thing I want to add is when it displays the witness_v0_keyhash scriptPubKey...I'd like it to decode and print the corresponding p2wpkh address...
< arubi> bsm117532, which address? the hash160(pubkey)?
< bsm117532> Yes. It's a simple change to ExtractDestination.
< arubi> bsm117532, fwiw, I vaguely remember adding "|| (whichtype == TX_WITNESS_V0_KEYHASH)" to 'ExtractDestination()' in standard.cpp to make that happen with decodescript, but really I just did it for debugging.
< arubi> ah yea ^
< bsm117532> I don't think we can know the destination for P2WPKH though since it uses a 32 bit hash...
< bsm117532> arubi: That's exactly what I just did, compiling now... ;-)
< arubi> it should work :), but I don't know if it's a sane thing to show a user.. it doesn't really mean anything
< bsm117532> Why not?
< arubi> because a version 1 address is not something a person that expects payment to a witness script should look for, even though it's probably done by default
< arubi> or, maybe it does have its uses
< arubi> I'm not sure. I thought maybe someone would want to sign a message with that key, but really the other side needs to know how to handle witness v0 too to parse that..
< bsm117532> I don't understand...the address in the scriptPubKey is the actual destination, and is consensus enforced, no?
< arubi> I don't understand the question, sorry
< arubi> what I mean is that a 1... address is a script, p2pkh. why would a person looking at a witness v0 script want to see a 1.. address?
< bsm117532> arubi: The withss_v0_keyhash address is the actual destionation, no? What would be wrong with printing the address in base58 instead of the hex hash160?
< arubi> there is no actual address for a witness v0 script, at least afaik
< bsm117532> Sure there is, it's that one in the scriptPubKey...unless I'm horribly confused.
< arubi> you mean 0x00 0x14 0x<hash160> ?
< bsm117532> Yes
< arubi> that has no address pattern that I know of, I might be wrong
< bsm117532> My tiny patch does this: https://www.zerobin.net/?ad6eb3578c41e566#Sy4WYHvXEqVGSD3+raknP1SfNUzN+KC9AsejLzSIReI=
< arubi> right, that's what I did.
< bsm117532> What's wrong with this address msFV...?
< arubi> it's a p2pkh script address
< bsm117532> So? it's the corresponding input that determines that a segregated witness needs to be used, not the address itself.
< arubi> it might help with referencing the private key, because that's how core works, but nothing else
< arubi> I agree that it has some value to a power user, but it's confusing at best to anyone else
< bsm117532> How could it possibly be confusing? Is there any chance that the privkey corresponding to msFV... does not control the sent funds?
< bsm117532> This scriptPubKey is the only indicator of the destination address...
< arubi> it's not about that, listing that address will make folks accidentally accept payments to both v0 witnesses /and/ that p2pkh script
< bsm117532> The new address types BIP for segwit was deferred. I don't fully understand that...
< arubi> there is no address type for v0 and v0 witnesses
< bsm117532> So some users are going to have segwit payments and their wallet doesn't know how to spend it, you're saying, because they didn't upgrade their wallet?
< arubi> er, v0/v1
< arubi> no one is going to accept payments to segwit scripts unless they actually know what they're doing, at least at first
< bsm117532> Sure
< arubi> bsm117532, the correct way now is to wrap it in a p2sh
< bsm117532> Well without this decoding step, `decoderawtransaction` gives absolutely no indication of the destination...which is why I want to decode it. :-P
< instagibbs> bsm117532, im working on a small PR to print out witness data when errors occur in signing
< bsm117532> arubi: FWIW I'm building an application that will be segwit-only.
< bsm117532> instagibbs: cool
< arubi> bsm117532, what do you need a 1... address for then? :)
< bsm117532> arubi: I'm rather confused. A 1...address could be either P2PKH or P2WPKH depending on how the sender encodes the witness data, no?
< arubi> no no
< arubi> a p2wpkh has no address type. when it gets one, /that/ should be listed in "addresses"
< arubi> bsm117532, a 1... address is the script "0x76 0xa9 0x14 0x<hash160> 0x88 0xac", it's "noise" coming out of decodescript for a v0 witness
< bsm117532> So I've been sending funds to P2PKH (hash160) addresses, with segregated witness, and it works fine!
< bsm117532> Oh I see what you're saying
< arubi> well a v0 witness has a scriptcode just like a p2pkh script, so it verifies the same
< bsm117532> arubi: No, a 1...address is a version byte concatenated with the hash160...
< bsm117532> and the segregated witness verifies the hash160.
< arubi> that's just the encoding, not the actual payment script
< bsm117532> Sure
< arubi> no, it actually does a p2pkh verification. hash(pubkey) == hash && scriptsig & pubkey
< bsm117532> Yes
< bsm117532> Yes the script is implicit.
< arubi> but not the address encoding
< arubi> the 1.. address encoding is not the correct output. an eventual v0 script address is
< arubi> v0 witness*
< bsm117532> So there is an intent to bring BIP 142 and new address types (eventually) then?
< arubi> I don't know. I hope there will be, so people could use it
< arubi> p2wsh(p2pk) is what you want now for about the same functionality
< bsm117532> To be clear, I can create and spend P2WPKH funds on testnet, now.
< instagibbs> bsm117532, sipa has been working on better address schemes. Not sure when he'll be "done" though :)
< bsm117532> I see
< arubi> yes, spend to bare sigs and redeem them, it's all possible. but a simple wallet couldn't send /to/ you
< arubi> bare scripts*
< bsm117532> FYI, `decoderawtransaction` DOES decode V0 witness addresses as I wanted to do above.
< arubi> now after the patch, or before? :)
< bsm117532> now
< bsm117532> Oh....
< arubi> it's amazing how we've now both walked the same path :P
< bsm117532> Hahaa you're right my patch changed that output.
< bsm117532> Ah, then, this is a problem. I need bitcoind to track successive P2WPKH spends...and it's not doing it: "Invalid or non-wallet transaction id". And, adding the P2PKH address doesn't help (thanks arubi, now I see why)
< arubi> cheers bsm117532
< bsm117532> I guess what I need to do is to get bitcoind to add a single txn to its txindex. (Or use P2SH embedding)
< GitHub69> [bitcoin] jonasschnelli pushed 2 new commits to 0.13: https://github.com/bitcoin/bitcoin/compare/ebea65121e6c...66dde4edf733
< GitHub69> bitcoin/0.13 ea91961 Chris Moore: Fix formatting error...
< GitHub69> bitcoin/0.13 66dde4e Jonas Schnelli: Merge #8382: Fix formatting error...
< GitHub22> [bitcoin] jonasschnelli closed pull request #8382: Fix formatting error (0.13...patch-4) https://github.com/bitcoin/bitcoin/pull/8382
< arubi> why not p2sh(p2wsh({p2pk,multisig})) ? much more versatile
< bsm117532> I'm trying to be efficient and this is a non-wallet usage. ;-)
< bsm117532> Also, learn segwit by fire...
< bsm117532> importpubkey also doesn't cause bitcoind to pick up pure segwit payments... :-/
< arubi> bsm117532, I wonder if it'd track it using createwitnessaddress
< arubi> well probably not
< bsm117532> I don't understand what createwitnessaddress is for...
< arubi> oh disregard entirely, you want to track p2wpkh
< arubi> it's for creating p2sh(p2wsh(..))
< bsm117532> Oh I see
< arubi> what about tracking using bip32? that's what I'm doing now so suddenly I think it's good for everything :)
< bsm117532> Well I know all the txids involved, the problem is that even with -txindex, bitcoind isn't indexing them, because they're not in my wallet!
< arubi> oh sure it will index them with txindex
< bsm117532> (so gettransaction doesn't work on these P2WPKH -> P2WPKH transactions)
< bsm117532> txindex indexes everything, not only things in your wallet?
< arubi> you should be using getrawtransaction <txid> 1 ( 1 if you want json)
< arubi> gettransaction is handled by the wallet, for wallet txs
< bsm117532> arubi saves my bacon, again
< arubi> :)
< bsm117532> "light bulb"
< GitHub167> [bitcoin] instagibbs opened pull request #8384: Add witness data output to TxInError messages (master...txinerr) https://github.com/bitcoin/bitcoin/pull/8384
< bsm117532> I can test that instagibbs, shall I tACK it? I'm new to the procedures...
< Chris_Stewart_5> sipa: Does what we were talking about earlier apply to int's? I'm looking at how transactions are serialized for signatures and I see this line
< gmaxwell> bsm117532: if you've tested and it works you can tested ack.
< bsm117532> will do
< instagibbs> I just tested the bip143 examples, so please test :)
< Chris_Stewart_5> If we have an 'int' that is 4 bytes it is serialized to 4 bytes, however if we have a number such as SIGHASH_ALL(1) it is serialized to '01'
< Chris_Stewart_5> specifically this is related to some of the tests in sighash.json with really large hash types
< Chris_Stewart_5> wait..
< Chris_Stewart_5> nvm
< bsm117532> Chris_Stewart_5: that's going to get serialized as a varint if nHashType is very large, isn't it?
< Chris_Stewart_5> bsm117532: No, it is serialized as a 4 byte integer, I'm confusing myself with something else :-)
< Chris_Stewart_5> bsm117532: I'm getting confused with actually checking the signature, which has to be an 'unsigned char' https://github.com/bitcoin/bitcoin/blob/master/src/script/interpreter.cpp#L1209
< * bsm117532> checks python-bitcoinlib -- which serializes as an *unsigned* int while bitcoind is using a *signed* int. I hope that never matters, but I'd better fix it.
< bsm117532> instagibbs: might as well add the empty-witness check to your PR. https://github.com/instagibbs/bitcoin/blob/txinerr/src/script/interpreter.cpp#L1320
< bsm117532> After that line adD: if (witness.stack.size() == 0) { return set_error(serror, SCRIPT_ERR_WITNESS_PROGRAM_WITNESS_EMPTY); }
< instagibbs> I'm not going to mix debug and consensus PRs ;P
< instagibbs> (even though it should be obviously correct)
< jtimon> instagibbs: no, use CValidationState and the debugging can be done after the faulied check
< jtimon> well, I don't know what you were discussing before, so maybe I got you out of context...
< jtimon> oh, debug and consensus *PRs*, never mind
< * jtimon> hides
< instagibbs> :P
< bsm117532> I see how it is, make *me* do a consensus PR :-P
< * instagibbs> hides
< Chris_Stewart_5> So if we have a hash type not explicitly defined as a SIGHASH procedure, it is casted to an unsigned char then appended to the signature?
< Chris_Stewart_5> and wherever the number ends up is the procedure that is used?
< sipa> Chris_Stewart_5: hashtype is an integer, so it is serialized as 4 bytes
< sipa> Chris_Stewart_5: yes, the hash type is one byte, thought it's treated as a 32-bit int in the signature hash
< sipa> when did segwit activate on testnet?
< btcdrak> sipa: Dec 31st
< btcdrak> oh wait i misread "segnet"
< btcdrak> #834624 on testnet
< sipa> btcdrak: thanks
< sipa> maybe we didn't really communicate clearly about that
< bsm117532> I'm puzzling over this transaction which appears as part of the tests in python-bitcoinlib: https://www.zerobin.net/?eb75da08cae8988f#I5FnTYeOK6+6lXtVfp9Wa0O+sZq/hNBhdXOE0zi0me0=
< bsm117532> If I read it correctly, it's got zero inputs and one output, and those are identical to segwit's marker and flag bytes.
< bsm117532> So, it should trigger segwit deserialization. But bitcoin does not trigger segwit deserialization and I don't see why...
< sipa> bsm117532: fundrawtransaction and decoderawtransaction first attempt to deserialize without witness support
< sipa> as those are the only functions that can have a transaction with 0 inputs
< bsm117532> Ah I see.
< bsm117532> Thanks. I think I'll change the test for this then. It's an example of an invalid transaction, and is still invalid for segwit deserialization reasons instead of missing-input reasons.
< bsm117532> It looks like this test was in Bitcoin around 0.9x and earlier but was removed.
< sipa> if i had known about the need for deserializing transactions with 0 input i'd have made the flag byte 0xFF instead of 0x0
< bsm117532> Has this been a problem for others?
< sipa> longer term, there is no need why non-complete-transactions actually need to use the same serialization at all
< sipa> and more complex multisig schemes will need a lot more metadata to be passed along betwee signers, before a valid transaction appears
< bsm117532> Good point...
< sipa> so we can just come up with a better serialization format for that later
< sipa> and get rid of that ambiguity that now exists
< bsm117532> I was just explaining to our intern why little endian exists. Someday interns will ask why every financial system has these weird marker and flag bytes in its transactions. ;-)
< gmaxwell> don't let them get anywhere near biology.
< GitHub51> [bitcoin] Hektve87 opened pull request #8385: Can't automatically merge (0.8...master) https://github.com/bitcoin/bitcoin/pull/8385
< GitHub29> [bitcoin] Hektve87 closed pull request #8385: Can't automatically merge (0.8...master) https://github.com/bitcoin/bitcoin/pull/8385
< jtimon> I have way too many PRs open, a link will be better
< jtimon> includes 2 from nicolas dorian (#8342 #8341) and from me (#8348 #8347 #8346 ) and also also passes Consensus::Params instead of calling Params()
< jtimon> should be easy to review