< fanquake>
Does anyone know why Travis gets marked as failing, even though all tests are passing? i.e https://travis-ci.org/bitcoin/bitcoin/builds/558768209 is all green, but I see red in the GitHub UI (#16386) .It seems that running the extended lint stage will kick it over to green though..
< bitcoin-git>
bitcoin/master 8f250ab Steven Roose: TEST: Replace hard-coded hex tx with classes
< bitcoin-git>
bitcoin/master 0822b44 MarcoFalke: Merge #15282: test: Replace hard-coded hex tx with class in test framework...
< bitcoin-git>
[bitcoin] MarcoFalke merged pull request #15282: test: Replace hard-coded hex tx with class in test framework (master...util-messages) https://github.com/bitcoin/bitcoin/pull/15282
< jb55>
stevenroose: it looks like it's serialized as compactsized-length and then a string, so no.
< stevenroose>
jb55: cool, thanks!
< stevenroose>
I need to store multiple pieces of information in there for some project. So I'll fill it with a JSON object with multiple fields :D
< stevenroose>
They say blockchain is just a slow database :D
< stevenroose>
Something else less core-related: the segwit p2wpkh verification is implemented totally stand-alone, right?
< stevenroose>
It's not converted into some weird script or so, right? I'm pretty sure it's not. It's just that I'm spending a segwit output in a test script and I'm getting this error:
< stevenroose>
non-mandatory-script-verify-flag (Script failed an OP_EQUALVERIFY operation) (code 64)
< stevenroose>
I'm asserting above that this is the script: 0014621daf5f171129d0c56f9d012b9306e215bde726, which is clearly a segwit script and not p2pkh or whatever script with OP_EQUALVERIFY
< stevenroose>
Does segwit also require adding the sighash byte at the end of the signature?
< sipa>
p2wpkh/p2wsh do
< sipa>
and p2wpkh uses as redeemscript the equivalent p2pkh script
< stevenroose>
sipa so for the bip143 sighash calculation, do I need to provide a "witness script"?
< stevenroose>
I thought that was only for p2wsh.
< stevenroose>
After adding the sighash byte, I'm getting this: non-mandatory-script-verify-flag (Signature must be zero for failed CHECK(MULTI)SIG operation) (code 64)
< stevenroose>
It seems that segwit operations somehow enter the script execution code, which confuses me.
< sipa>
stevenroose: the witness script never goes into the sighash
< sipa>
not in v0 witness or in legacy
< sipa>
the *executed code* (scriptcode) goes into the sighash
< stevenroose>
sipa: this field 5. scriptCode of the input (serialized as scripts inside CTxOuts)
< stevenroose>
Is called "witness_script" in Andrew's Rust implementation.
< sipa>
that's confusing
< stevenroose>
What should I provide for that? The output script of the utxo?
< sipa>
the executed code
< sipa>
for p2wsh that is the witness script
< sipa>
for p2wpkh that is the equivalent p2pkh script
< stevenroose>
I don't know what the "executed code" or "witness script" refer to?
< stevenroose>
Witness program?
< sipa>
bip143 has examples
< stevenroose>
The p2wpkh script_pubkey?
< sipa>
the program being executed
< sipa>
i don't know how i can be clearer :)
< stevenroose>
ss << static_cast<const CScriptBase&>(scriptCode);
< sipa>
yes
< stevenroose>
sipa: well I don't know which program is gonna be executed without knowing the internals, right? :) What is more helpful is where I can find that program..
< sipa>
scriptCode is the script being executes by the script interpreter
< sipa>
for p2wsh that is the witness program
< sipa>
for p2wpkh that is the equivalent p2pkh program (dup hash160 <hash> equalverify checksig, iirc)
< stevenroose>
I'm getting OP_EQUALVERIFY errors, so I'm getting the feeling that segwit validation is somehow done by converting the witness program into a legacy script equivalent. I never heard of such a thing, though, so I'm confused :)
< sipa>
yes
< sipa>
i just told you
< stevenroose>
Ah ok, so there actually *is* some kind of p2wpkh->p2pkh convertion under the hood.
< sipa>
yes
< stevenroose>
Wow, that's strange :) Glad that goes with taproot :) Ok, so I have to serialize a legacy p2pkh output script and provide that. Cool, thanks!
< stevenroose>
Yay, that worked!
< sipa>
:)
< Raystonn>
There are way too many cases of CAmount and double being mixed together. That's just unsafe. I'm going to start cleaning those up. CAmount should not be used with floating point types, which can drop digits.
< sipa>
doubles have 52 bit precision, which is sufficient for any valid BTC amount
< sipa>
internally, amounts are always represented as CAmount
< Raystonn>
That makes assumptions as to the internal representation of CAmount.
< sipa>
CAmount is an int64
< Raystonn>
This can change in the future.
< sipa>
sure
< Raystonn>
Test cases that mix double with CAmount will then start failing.
< Raystonn>
Not great.
< Raystonn>
The code shoudl be more robust there. I will clean it up.
< sipa>
internally everything is represented as CAmounts
< sipa>
floating point things are only used for feerate policies
< Raystonn>
I wish that were true.
< Raystonn>
Lots of mixing in double calculations in core code.
< Raystonn>
decays, etc.
< sipa>
where?
< sipa>
the fee estimation code, sure
< sipa>
consensus rules only use CAmount
< Raystonn>
That's a great start.
< Raystonn>
and certainly required.
< sipa>
if you were more specific i could give better advice
< sipa>
you're not going to rewrite the fee estimation code without floating point logic
< Raystonn>
It's certainly possible. One can break out such code into integer operations that use integer numerator and denominators instead of a single float decay rate. A CAmount can then be multiplied by the numerator, then divided by the denominator, and still get the desired result. The fractional portion of the result will still be truncated when storing the actual fee as a CAmount.
< sipa>
yes, you can
< Raystonn>
This would be safe even if CAmount was upgraded to a larger-range type.
< sipa>
but why would you?
< sipa>
exact accuracy isn't required for any of that
< Raystonn>
Whereas use a a floating point type will break the calculations upon any such upgrade in the future.
< sipa>
CAmount can't be upgraded to a larger range type
< Raystonn>
Of course it can.
< sipa>
and even if it did, nothing would break with feerate calculations
< sipa>
as those are approximate anyway
< Raystonn>
It could easily overflow the significant digits available in a double if we upgrade to a 128-bit integer.
< sipa>
So?
< sipa>
a 0.0000000000002% rounding error on a feerate is much less than the variations that are inherent due to inconsistency of mempools
< sipa>
and again, bitcoin's consensus rules don't permit amounts above 2100000000000000 units
< Raystonn>
If fees were to move into sub-satoshi range, that rounding errors would be pretty large as everything sub-satoshi gets truncated.
< sipa>
bitcoin does not have sub-satoshi units
< Raystonn>
yet
< sipa>
that would be such an invasive hard fork that the code changes necessary to do it correctly will be the least of our worries
< Raystonn>
and there's no erason to keep code that woudl break under these circumstances when it's so easy to fix.
< sipa>
please focus on real issues
< Raystonn>
I'm not asking you to make any changes.
< Raystonn>
This is what I'm choosing to look at.
< sipa>
and i'm suggesting there are better ways to spend your time
< Raystonn>
That's fine.
< Raystonn>
We can disagree. ;)
< sipa>
of course
< sipa>
but in my opinion, getting rid of floating point logic in places where exact accuracy isn't required would be a waste of your time
< Raystonn>
Normally I'd agree. But upon testing a change of CAmount to boost::multiprecision::int128_t, the floating point operations aren't even supported. I could add them, but it could lead to dropped precision or accuracy some place where it matters simply by defining the operators.
< sipa>
there is no need for 128 bit integers
< Raystonn>
Right now no.
< sipa>
it seems highly unlikely that a change to the monetary policy will ever happen in bitcoin
< sipa>
you can prepare for all kinds of theoretical adaptations, but this isn't a realistic one
< Raystonn>
Bitcoin must remain <= 21 million coins.
< Raystonn>
Divisibility of those coins can be expanded.
< sipa>
not without a very invasive hardfork
< sipa>
and such changes are generally considered off-topic here
< Raystonn>
Right now I'm just talking about cleaning up use of floating point operations where unneeded.
< sipa>
they are not unneeded
< sipa>
they're an engineering choice
< sipa>
yes you can get rid of them at the cost of replacing them with more complex logic that effectively reimplements it
< sipa>
but that is not a good trade-off
< sipa>
there is nothing "unclean" about the use of floating point numbers; they're the right tool for the job, in some places
< sipa>
and i can't speak for any of the other reviewers of course, but i doubt such a change would be accepted
< sipa>
now if you find places where floating point logic is used where inaccurancy may actually affect the correctness of the code, i'd very much like to hear it
< Raystonn>
I certainly have no interest in any fork that might create yet another sh-coin. Not my intention. I'm just trying to make the code more robust to future enhancements without breaking backward compatibility.
< Raystonn>
Many years into the future I could foresee the desire to continue the block reward into sub-satoshi units, keeping the asymptote at 21,000,000 coins.
< sipa>
that would be a change to the monetary policy
< sipa>
and again, i don't see how that would in any way invalidate the use of floating point for feerate calculations
< Raystonn>
For a code path that has never been executed, and won't for at least 120 years.
< sipa>
it is executed every block
< sipa>
so please, if you want to contribute in a useful, focus on other issues
< sipa>
not long-in-the-future hypotheticals that you still haven't clearly motivated would invalidate the current code
< Raystonn>
I mean the code path that would be added to support sub-satoshi block rewards... we wouldn't get there for another 120 years.
< achow101>
Raystonn: please keep in mind that any change you make must be reviewed by others and they must all approve it before a change is merged. It is highly unlikely that such a change will pass review, so I would recommend that you don't waste effort into trying
< Raystonn>
Anything added there now would never see a fork.
< sipa>
Raystonn: sorry, hardfork discussions are offtopic here (you can bring that up on the mailinglist if you want)
< achow101>
Raystonn: if we wouldn't gt there for another 120 years, then do it 120 years in the future when it becomes a problem
< Raystonn>
I will drop the discussion on sub-satoshi block rewards.
< Raystonn>
120 years from now it would result in a fork as people are already running the code and getting no reward.
< bitcoin-git>
[bitcoin] achow101 opened pull request #16394: Allow createwallet to take empty passwords to make unencrypted wallets (master...fix-born-enc) https://github.com/bitcoin/bitcoin/pull/16394
< stevenroose>
Is there a flag for the min tx fee to include in blocks? Equivalent to minrelaytxfee?
< stevenroose>
-blockmintxfee?
< luke-jr>
right
< elichai2>
achow101: Hey, any insight of the reason that `createpsbt` need to support 2 different formatting for the outputs? (one as an array another as a dictionary)
< achow101>
elichai2: createrawtransaction allows both (api changed at some point, but old style kept for backwards compatibility), and the same construction function is used in both so createpsbt takes both
< elichai2>
:/ makes it harder to add new stuff there, but i'll figure it out, thanks!
< sipa>
elichai2: why would you need to touch that rpc?
< elichai2>
sipa: testing how would taproot look when combined into psbt
< sipa>
you shouldn't need to touch any of those rpcs
< sipa>
only the signing/psbt logic
< sipa>
and descriptors
< elichai2>
sipa: you're right. but if I go the regular way I need to add: 1. new descriptors. 2. classes and support for WitnessV2 bech32 addresses. 3. Classes for taproot in the wallet. serialization+deserialization of taproot and witness v2 addresses.
< elichai2>
this is a lot of logic that require more careful handeling
< elichai2>
and way more work. I'm trying to concentrate around PSBT for now. so i'm using `createpsbt` as a "hack" because that's the only psbt RPC command that doesn't relay on the wallet. (I'll probably move that logic to a seperate "createrawpsbt" or a `bitcoin-psbt` bin)
< sipa>
for experimentation purposes that's fine of course
< sipa>
but i don't think we'll add any taproot support to the wallet except "doing it right"
< elichai2>
yeah I get the separation of concern for the psbt RPC methods
< sipa>
no, i mean: nothing in those RPC arguments should change for taproot
< elichai2>
sipa: does "doing it right" require full taproot support in the wallet? or can it be just psbt support via a raw psbt creation method that doesn't require a wallet?
< sipa>
elichai2: i don't see how those two options are different :)
< achow101>
elichai2: it would mean full support in the wallet with walletprocesspsbt handling all of the updating in the background. i.e. the user never provides the taproot info in an rpc
< sipa>
if we have taproot support in descriptors and psbt, we'll automatically have it in tbe wallet
< achow101>
just like right now the user never provides keys, utxos, etc. in the rpc
< elichai2>
achow101: yes. which makes it harder to use if for example this isn't a transaction that you have in your wallet. i.e. there's no equivilant to `createrawtransaction`
< sipa>
there is createpsbt?
< achow101>
but that's a problem with updating any psbt right now
< elichai2>
achow101: right
< elichai2>
solving that problem will make adding taproot to psbt easier by not needing to add it to the wallet first
< sipa>
what does "adding it to the wallet" mean?
< elichai2>
elichai2 sipa: you're right. but if I go the regular way I need to add: 1. new descriptors. 2. classes and support for WitnessV2 bech32 addresses. 3. Classes for taproot in the wallet. serialization+deserialization of taproot and witness v2 addresses.
< sipa>
and we have psbt support for the full stack of operations outside of the wallet
< sipa>
taproot can be implemented tested without ever touching the wallet code at all
< achow101>
*assuming descriptor wallets
< sipa>
maybe there are some minor changes at some point past descriptor wallets to change the default to taproot addresses or something
< elichai2>
sipa: what achow101 said
< sipa>
elichai2: well, not even
< sipa>
ah, right, we don't have the equivalent of walletprocesspsbt using descriptors yet
< sipa>
maybe we need that first :)
< sipa>
then all we need is adding taproot to psbt/descriptors, and the full stack of operations would be supported outside of the wallet
< sipa>
musig is harder though, as it requires state on the signer device
< elichai2>
as far as I could see, right now if you'll pass a bech32 with segwit v2 it won't know how to parse it. it won't know that the witness program contains a public key. and it doesn't have any structure to handle a tree with multiple scripts
< sipa>
yes it does
< sipa>
(the first thing)
< sipa>
i don't think we need a whole tree structure even
< stevenroose>
Is there a way to "reset" the Core wallet? I'm using regtest using a client and in between tests, I'd like to sweep the wallet from all imported addresses and transactions.
< sipa>
elichai2: just a psbt record of the form "this pubkey is derived from this internal pubkey, and this merkle branch, to this script and leaf version"
< stevenroose>
I tried to unload the active wallet and "createwallet" a new one, but then it expects me to use the /wallet/<walletname> endpoint, which the client we're using doesn't support yet.
< elichai2>
sipa: for a wallet support we need the whole tree structure, for psbt we don't
< sipa>
elichai2: the tree would be in the descriptor
< sipa>
nowhere else
< sipa>
the 32-byte x coordinate idea (which we may update taproot with) may affect how that psbt record is structured too
< elichai2>
sipa: correct me if I'm right, but trying to give it bech32 with witnessV1 will make it use `WitnessUnknown`, right? so we need some struct like `WitnessV1PubKey`
< sipa>
yeah
< sipa>
that's easy :)
< elichai2>
sipa: yeah, that's a small change that could be easily changed too :)
< sipa>
but it's not like we need new parsing code; bech32 parsing is alreasdy generic
< sipa>
elichai2: i suspect you're overestimating how much work it is to adapt all the structures with taproot
< sipa>
it's nontrivial of course
< sipa>
but i think hacking in via additional RPC arguments in various places won't be all that much simpler
< elichai2>
i'm just trying to figure out if I could make a PoC without while minimizing the areas of the code I'm touching, because I don't know most of the code yet
< sipa>
though it will touch a lot more different parts
< elichai2>
because if i'll start editing code all over i'll probably mess things up even more lol
< elichai2>
I understand that my PoC won't be close to optimal. and that's why I want it for now to be isolated into an RPC, and not doing all of the wallet support
< sipa>
so why do it in bitcoin core at all then?
< elichai2>
because I still think that manual psbt without descriptors is still useful? altough you're right that I could've done it in rust-bitcoin and it would probabaly been easier and more native to the code but in the end I do want to get to know bitcoin core's code better so I will be able to make optimal solutions to bitcoin in the future :)
< sipa>
well if you want to learn bitcoin core better (which i very much encourage you to!), i think it's better to focus on one piece at a time, but actually integrate it
< sipa>
for example, an RPC that takes a descriptor and a bunch of private keys, and signs with them, would be pretty generally useful
< sipa>
and for the extensions to psbt... i think it's generally too early
< elichai2>
sipa: my hope is that if people start building things on top of taproot it will increase the chance of merging and activation sooner rather than later
< elichai2>
so why not start conversations around descriptors extensions, psbt extenstions, tools that build complex taproot trees, early integration into other wallets, ideas on how to use it to make cheaper transactions, etc.
< elichai2>
but I might be totally wrong
< sipa>
i don't know, it feels like unnecessary pressure
< sipa>
we first need to get agreement on a design
< elichai2>
maybe this will make more people interested in reading into taproot BIP and giving feedback?
< sipa>
i'm scared of people building things, and then discovering a major change in necessary, and then we end up with invested effort driving a "pfff please don't change the design anymore, we'll need to redo our code!", which is a really bad incentive
< elichai2>
but I agree that if the design will really change then maybe this will be a big waste of time
< sipa>
at a high level, i think all of these things are easy... there is a lot of engineering work to go into making it actually work
< sipa>
but once taproot is finalized, i think the way to actually add it to psbt for example will be pretty straightforward
< elichai2>
sipa: now I understand your motivation for discouraging a lot of the things I said that are related to taproot :) thanks :)
< elichai2>
sipa: how much time it took between segwit activation and full support in core's wallet? I might really be off here but for me it felt a lot
< sipa>
elichai2: yeah, a lot, and i think that's perfectly fine :)
< sipa>
but i suspect with psbt and descriptors it will actually be a lot easier
< elichai2>
don't you think that bitcoin core wallet should be like a reference for other wallets? because in that case other wallets got it faster :/
< sipa>
i don't think so
< sipa>
we need reference code in the form of signing logic etc
< sipa>
and test vectors and examples
< sipa>
but the variety of things that can be done with taproot is so large... there really isn't a way to be reference for everything
< sipa>
actually, it was only from august 2017 to feb 2018
< elichai2>
as far as I could tell in the code psbt is basically a big wrapper(not trying to underestimate the power of it) around the already existing serializations of the classes/structs
< sipa>
yup
< sipa>
so what will be needed in terms of data structures is a "taproot derivation" record in signing providers
< sipa>
and descriptors that can fill that record
< sipa>
and a way for that record to be serialized in psbt
< stevenroose>
(nvm my question, btw :))
< sipa>
and signing logic to use it
< elichai2>
stevenroose: sorry for spamming it out 😓
< sipa>
stevenroose: delete the file :p
< elichai2>
sipa: I might need to look into the descriptors code, but basically I wrote a draft BIP for the PSBT extensions and wanted some PoC, so I thought adding manual RPC method for PSBT will be the easiest place to add those extensions without understanding all the structures in the wallet
< elichai2>
But you might be right and I need to look more into descriptors
< elichai2>
And my way isn't "the right way" in relation to how the current code is designed
< sipa>
i think for descriptors we'll probably want a fragment "tap(KEY,[[X,Y],[Z,[T,U]]])" kind of construction, where you give the root key and the leaves in some tree-encoding way
< sipa>
where the X/Y/Z/T/U are subexpressions
< elichai2>
and you need the script too
< sipa>
X,Y,Z,Z,T,U are subexpressions that represent scripts
< sipa>
they could be pk(), or multi(), ...
< sipa>
or once we have miniscript probably a whole bunch more things (i hope to publish more about that soon)
< elichai2>
oh that's for the whole tree. not just the spending path
< sipa>
right
< elichai2>
I'll more need to look into the descriptors code, see how complicated is it
< sipa>
and that would then get converted into a "taproot key record" for the key path and for each leaf
< sipa>
L149-170 is an interface for all the "key" expressions inside descriptors, with a number of implementations (pubkeys, descriptors, origin info)
< sipa>
s/descriptors/bip32/
< sipa>
line 335-497 is a generic implementation of a "script" node in a descriptor
< sipa>
followed by implementations that add node specific logic (pk, pkh, sh, wpkh, wsh, multi, combi, addr, raw)
< sipa>
and after that is parsing (string to descriptor) and inference (script to descriptor) code
< elichai2>
sipa: Thanks. I'll start looking around
< sipa>
elichai2: cool
< elichai2>
(really, Thank You!)
< sipa>
really, yw :)
< jnewbery>
sipa: you've mentioned this concern a couple of times:
< jnewbery>
> i'm scared of people building things, and then discovering a major change in necessary, and then we end up with invested effort driving a "pfff please don't change the design anymore, we'll need to redo our code!", which is a really bad incentive
< jnewbery>
I think there are a good reasons to start building PoC tools around the schnorr/taproot proposal now.
< jnewbery>
We'll get much better feedback on the design if people are actually writing code to use schnorr/taproot, rather than just trying to analyse it from a theoretical perspective.
< jnewbery>
And if those tools are made in a flexible way such that they can be adapted if there are minor design changes, then I don't think it's a waste of engineering time.
< jnewbery>
I can't think of what kind of major change to the proposal would cause the effort to be wasted.
< sipa>
jnewbery: of course
< sipa>
i don't mind PoC tools
< sipa>
but i also don't think there is any rush
< sipa>
and if your end goal is integrating things into bitcoin core, i suspect getting familiar with the code is probably the best way to spend time now
< jnewbery>
I don't think there's a rush, but I'd prefer for these tools to be built now so we can get feedback on the proposal. We're now at the stage that we're gathering feedback on the proposal, so it seems like a good use of time/energy to try to encourage that feedback.
< jnewbery>
and yes, getting familiar with the code is important, but I don't think those things are mutually exclusive in any way
< sipa>
i don't disagree
< sipa>
but do you think hacking in preliminary taproot code in RPCs where it shouldn't be needed is useful as a PoC/
< sipa>
it's not useful as reference code, and is unlikely to give a good idea of the complexity of real world implementations
< sipa>
if a PoC is your goal, i suspect there are better codebases to try implementing things in that core's
< sipa>
and if working towards an eventual production implementation in core, time seems better spent on the actual code rather than bypassing it
< sipa>
*than
< jnewbery>
> do you think hacking in preliminary taproot code in RPCs where it shouldn't be needed is useful as a PoC?