< achow101>
elichai2: to not publicly expose the internals
< elichai2>
hmm
< elichai2>
ok, I guess everything I add to `FlatSigningProvider` I need to add to `FillableSigningProvider` but thread safe
< achow101>
Why do you need FillableSigningProvider?
< elichai2>
achow101: right in FlatSigningProvider everything is public
< elichai2>
achow101: I added taproot support for descriptors, it complains now that the `switch (whichType)` in `IsMine` doesn't cover the new taproot
< achow101>
don't use IsMine :)
< elichai2>
I don't use it, I edited `txnouttype`
< achow101>
(but seriously, IsMine is changing with all of the wallet stuff)
< elichai2>
so for now I should just stick to descriptors and then enjoy the rebase? haha
< achow101>
I don't quite follow what you are trying to do
< elichai2>
I guess i'll have to manually test that I'm hashing everything the same way the interperter does
< achow101>
right, so what does ismine have to do with this?
< elichai2>
ismine switches over `txnouttype`. I had to add `TX_WITNESS_V1_TAPROOT` to it because `txnouttype` is also used in `InferScript`
< elichai2>
I can just add return false there for the taproot ones for now
< sipa>
you should
< sipa>
the old ismine logic shouldn't be used for taproot stuff
< elichai2>
so I'll wait for achow101 native descriptors PR for testing wallet support?
< achow101>
yes
< elichai2>
ok
< sipa>
are you writing signing support?
< elichai2>
I'll start doing manual tests to check that i'm hashing everything correctly
< sipa>
or just derivation for now
< elichai2>
the only way to really know that what i'm doing is correct is by having a test that signs and see that it passes your interperter code
< elichai2>
otherwise my lex ordering or the hashes might be wrong and I won't know
< sipa>
so signing logic should be independent of descriptors
< sipa>
and there you'll need to switch based on txnouttype
< sipa>
but for pretty much everything else i think you can ignore TX_WITNESS_V1_TAPROOT
< elichai2>
yeah, but then I can import a desc and try to sign with it. but I guess i'm trying to do too much, for now i'll test manually and concentrate on adding more tests for the desc and then either ask someone to give my code a quick look (I am pretty new to bitcoin core's code and to c++) or look into PSBT/signing, but only after i'm 100% done with the desc
< elichai2>
(hopefully by then achow101 will finish and I'll be able to rebase on top of him)
< sipa>
are you adding fields to SignatureData ?
< achow101>
elichai2: you could base on top of #16528 (native descriptor wallets), but it's wip so it could change. but while no one has reviewed it, it probably won't change
< elichai2>
I'll actually try to review it too next week, though it's a pretty big one heh
< elichai2>
sipa: didn't yet but I basically have the control block in ProduceSignature
< elichai2>
so it shouldn't be too hard
< sipa>
elichai2: i guess the codebase (and the hairy signing logic + descriptor stuff in particular...) can be quite daunting
< sipa>
but it's great that you're getting accustomed to it
< elichai2>
yeah, even though there's a lot of things I don't like/don't get both in c++ and in some design choices it's overall not as hard as I thought it will be (assuming people won't say that everything i'm doing is wrong and bad lol)
< sipa>
yeah, a lot is historical
< achow101>
it's all sipa's fault
< elichai2>
lol
< sipa>
i think maybe at some point we can make the entire signing logic integrated into descriptors
< sipa>
well, a lot of it is, probably
< elichai2>
yeah a lot of the historical stuff are frustrating, but I guess we don't have much of a choice
< elichai2>
hmm so in theory if you use '(' it would hopefully magically work with my desc
< achow101>
that will be part of the descriptor module? so the wallet doesn't need anymore changes
< elichai2>
altough you also use `,` so that might be a problem hmm
< sipa>
elichai2: i expect things to just Just Work(tm)
< sipa>
achow101: yeah
< sipa>
the only change that affects the wallet i expect will be that we can't use IsSolvable/DummySigner anymore to guess fees
< elichai2>
looks pretty cool :)
< sipa>
because the size of satisfactions depends on which participants are available
< sipa>
so i think we'll add a method to descriptors to ask them what a satisfaction will cost
< sipa>
that's also true for taproot btw
< elichai2>
I'm kind of brute forcing IsSolvable(ProduceSignature) right now, so if the wallet can either 1. sign for the internal key. 2. sign for any of the branches I return cool, which I guess will stop at the first thing that it can sign for
< elichai2>
though I'm not sure it's the right attitude. maybe at the very least sort it by size and try from lower to higher
< sipa>
elichai2: my idea is that we add support in descriptors for marking keys as available/unavailable
< elichai2>
available as in available but not in my wallet? (i.e. multisig)
< sipa>
and there is a cute algorithm that can find the worst case satisfaction cost given which keys are certainly available, which may be available, and which are definitely not available
< sipa>
yeah, available meaning "will participate in signing if requested"
< sipa>
which private keys you have locally should be pretty much completely independent (as your software shouldn't treat having a key locally as different from it being in a hw wallet you control)
< elichai2>
right now the wallet doesn't do anything like that for multisig, right?
< sipa>
right
< sipa>
because it doesn't need to
< sipa>
all satisfactions are the same size
< elichai2>
but some keys might be available but not in the wallet
< sipa>
yeah, currently it distinguishes
< sipa>
that's bad
< sipa>
the concept of "watch only" being tied whether you have keys locally is a mistake
< achow101>
watchonly is no longer a concept in descriptor wallets
< sipa>
yay
< achow101>
other than watchonly wallets, i.e. disable private keys
< achow101>
but ISMINE_WATCHONLY isn't a thing that descriptor wallet ismine can return
< elichai2>
so how will watch only work with native desc wallet?
< emilengler>
elichai2: TL;DR can you sum it up please :)
< elichai2>
emilengler: sum up what?
< emilengler>
elichai2: You've written if anyone is interested on what? Sorry didn't followed the conversation
< achow101>
emilengler: that's the code he's working on
< elichai2>
oh haha, this is my WIP for taproot descriptors
< achow101>
elichai2: watchonly in native descriptor wallets just means that the wallet doesn't have any private keys at all
< emilengler>
elichai2: ok cool, good luck
< elichai2>
achow101: how will you import such thing? just as an address?
< elichai2>
emilengler: thanks :)
< achow101>
even still, nothing is ever explicitly labeled as watchonly like how it is with legacy wallets
< achow101>
e.g. right now, if you import an address, ismine will return ISMINE_WATCHONLY
< achow101>
in native descriptor wallets, it will return ISMINE_SPENDABLE since ismine in native descriptor wallets is really just a bool
< gwillen>
with multiwallet, I have claimed and continue to claim that the most reasonable thing is to have spendable and watchonly wallets be completely separate wallet.dat files
< gwillen>
then there is no need to dispute over what "watch only" means because you pick
< sipa>
gwillen: yes, that's what's happening
< sipa>
the concept of watch only goes away with descriptor wallets
< achow101>
elichai2: there's an importdescriptors RPC to import things into descriptor wallets. the rest of the import RPCs are disallowed for them
< sipa>
something is treated as ours, or not
< gwillen>
so then the answer to "how does watchonly work" is "any way you want to, you name one of your wallets watchonly.dat and then import whichever keys you want there"
< achow101>
gwillen: pretty much. right now descriptor wallets is not allowing a mix of having private keys and not having them. I'll probably change that before it goes out of WIP
< gwillen>
(I think if you import "just as an address", you can watch, but you won't be able to prepare transactions for offline signing, which would require public keys, redeemscipts, etc. in addition to addresses)
< elichai2>
sipa: unrelated to anything. rebasing a branch with your subtree commit in it is hell lol (which is why I ended up merging instead)
< sipa>
elichai2: oh you can't rebase subtrees
< sipa>
i always just redo them
< elichai2>
sipa: oh really? so when you'll open the taproot PR you'll redo those commits?
< gwillen>
(but you can choose which way you do it dependding on what the particular wallet's function is for you)
< sipa>
elichai2: or anytime i rebase
< sipa>
they're trivial anyway
< achow101>
sipa: with miniscript and taproot, is the expectation that the wallet will have some of the keys for satisfying?
< sipa>
achow101: same as multisig
< sipa>
i don
< sipa>
i don't think we should care whether the user has keys locally or not
< sipa>
if he has any, it'll likely not be all of them
< elichai2>
ha. I tried to do it using rebasing. I tried every trick in the book lol so I gave up. when my commits will be more than a WIP i'll rebase them on top of a manual thing instead of a rebase
< sipa>
i'll do a rebase of my taproot branch soon
< elichai2>
that's fine as long as I don't publish my code there's nothing wrong with it being on top of a merge commit
< achow101>
sipa: right now in descriptor wallets, if we have private keys enabled, we require all of the private keys to be available in order to import a descriptor. that's probably wrong, but I haven't had the chance to really think through it
< sipa>
achow101: yeah i think that's wrong
< sipa>
though it may make sense to configure the wallet to be "strictly single key, all keys present" or so, as a sanity check
< sipa>
another possible setting for sanity checking could be "have at least one participating key in every descriptor"
< elichai2>
achow101: right now as in your pr or as in master?
< sipa>
elichai2: descriptor wallets only exist as a PR
< achow101>
I was unsure whether to allow importing a descriptor that had to priv keys to a privkey enabled wallet
< achow101>
s/had to priv keys/had no privkeys
< elichai2>
sipa: right. that was a sanity check because I don't remember any privkey stuff in the descriptors so I wasn't sure what's up
< elichai2>
achow101: hmm maybe it should be have enough keys to sign for that desc?
< sipa>
elichai2: that's uninteresting
< achow101>
the two ideas I was considering were "enough to sign" and "have at least one key"
< sipa>
if you have a sufficient number of keys locally to sign, you shouldn't be using anything but single key
< achow101>
right
< achow101>
so I think I'll go with the latter
< sipa>
that makes sense
< elichai2>
hmmm. so after you create the pubkeys map you make sure that you have at least one priv key? (or when you enter them into the map you could just set a bool)
< achow101>
elichai2: basically
< achow101>
haven't actually thought through the implementation
< elichai2>
cool. when do you think the PR will be ready for review?
< achow101>
It's a meme about Blizzard and Valve taking forever to release video games
< achow101>
"Soon™: Copyright pending 2004-2019 Blizzard Entertainment, Inc. All rights reserved. "Soon™" does not imply any particular date, time, decade, century, or millennia in the past, present, and certainly not the future. "Soon" shall make no contract or warranty between Blizzard Entertainment and the end user. "Soon" will arrive some day, Blizzard does guarantee that "soon" will be here before the end of time. Maybe. Do not m
< achow101>
ake plans based on "soon" as Blizzard will not be liable for any misuse, use, or even casual glancing at "soon.""
< sipa>
it
< sipa>
it's older than blizzard
< elichai2>
ohhh
< elichai2>
I googled `tm` and I actually found the tarkemark but didn't get the joke lol
< achow101>
elichai2: native descriptor wallets depends on the rework pr (#16341) to be merged first, so it will be ready only after that happens
< gribble>
https://github.com/bitcoin/bitcoin/issues/16341 | Introduce ScriptPubKeyMan interface and use it for key and script management (aka wallet boxes) by achow101 · Pull Request #16341 · bitcoin/bitcoin · GitHub
< achow101>
given that 16341 is really big, it might take a while
< elichai2>
ok, so I guess I'll try to review that one on Monday :) (though I don't know too much about the wallet itself)
< fanquake>
achow101: "Descriptors" with a horrible yellow colour
< emilengler>
Where are the blocks being written to the disk (file, function)?
< emilengler>
And how do the blocks are even stored? An urban hymn says through leveldb
< emilengler>
So is there a concret place in the src where the actual writing process happens?
< achow101>
emilengler: somewhere in validation.cpp
< emilengler>
achow101: thanks
< achow101>
blocks are written to the blk*.dat files in the blocks/ folder. leveldb is used for the indexes and other databases
< achow101>
emilengler: look at AcceptBlock, FlushStateToDisk, and FlushBlockFile
< achow101>
and SaveBlockToDisk
< emilengler>
Ok, I will check it :)
< emilengler>
By the way what is the AppVeyor thing and is it normal that it takes forever to finish?
< sipa>
it's a windows CI
< emilengler>
It might be buggy, the status on a GitHub PR which I started a month ago isn't being updated even the build was a success
< sipa>
it absolutely is
< sipa>
:)
< emilengler>
Good to know :D
< emilengler>
No wonder it's windows...
< mryandao>
is there a reason why lots of the logic going on in validation.cpp does not get refactored out into other files?
< sipa>
mryandao: because refactoring is hard
< sipa>
especially consensus critical code
< mryandao>
I see.
< emilengler>
Refactoring can often lead to some new bugs. Bugs in the consensus are a HUGE problem. A refactor of it would probably take lots of testing and reviewing
< sipa>
there are always efforts to refactor things here and there
< sipa>
over time lots of code has moved
< achow101>
i remember good ol main.cpp
< sipa>
in 2010 there was main.cpp which contained all of what is now validation/net_processing/net/wallet/protocol/coins/pow/...
< emilengler>
I never saw it but the file where the main class is, is mostly bloated. Know this from my personal projects. It might be a curse of C++...
< emilengler>
Bloated at the beginning at least
< mryandao>
i believe at 0.12 it was a bloated main.cpp
< mryandao>
that is not too long ago
< achow101>
checkout some of the older tags, you'll see it there
< mryandao>
the hong kong agreement drama times
< achow101>
main.cpp was only split up and removed in 0.14
< emilengler>
I'm currently looking at v0.3.0
< emilengler>
It is bloated in terms of v0.3.0 but not in terms of v0.18 :P
< emilengler>
the main file^^
< mryandao>
i hear poker logic existed in 0.1 or something
< sipa>
well, the code gained a lot of functionality since then
< mryandao>
maybe somebody here can verify
< sipa>
it's unfair to compare total complexity; of course it'll be more complex now
< achow101>
mryandao: checkout v0.1.5 and look for yourself
< achow101>
I seem to remember that at one point in time that main.cpp was large enough (and github shitty enough) that it wouldn't be fully rendered
< sipa>
yeah
< emilengler>
<joke>If it hadn't been splitted up, it would be a git lfs file today</joke>
< mryandao>
v0.1.5 is so minimalist
< mryandao>
i like.
< mryandao>
could maintain with just vanilla vim without plugins
< achow101>
it's windows and gui only, so you wouldn't be using vim to dev it
< mryandao>
there's gvim on windows
< emilengler>
Is vim that bad on windows?
< elichai2>
Windows is that bad period.
< emilengler>
ACK
< achow101>
well vanilla vim doesn't exist on windows
< emilengler>
achow101: What do you mean by this
< mryandao>
i cant find poker in v0.1.5
< emilengler>
mryandao: IIRC the poker was only a GUI thing
< emilengler>
You could try to open the project with a wxWidgets editor
< emilengler>
Then you might find something
< mryandao>
oh, i see it now.
< mryandao>
thanks.
< mryandao>
lol, what a meme.
< achow101>
I thought poker was pre-release
< emilengler>
achow101: A friend of me is compiling vanilla vim for windows around every 2 days
< achow101>
emilengler: TIL
< emilengler>
There is also a real vim binary for windows (Not the gui stuff)
< achow101>
I didn't know that the vim project actually published windows releases
< emilengler>
IIRC they still don't
< emilengler>
You need to compile it on your own or use pre-compiled online binaries from others
< emilengler>
But they support windows in terms of cross-platform
< emilengler>
Same with Bitcoin Core and FreeBSD for example
< achow101>
ah
< achow101>
fun fact, bitcoin 0.1 was released before Windows 7
< emilengler>
achow101: You're right but I never thought of it
< emilengler>
It was probably compiled on Windows XP because the screenshots from satoshi had this old winxp theming
< joseph-dev>
Hello?
< bitcoin-git>
[bitcoin] mmachicao closed pull request #15137: Tests: Contract test for CCoinsView and CCoinsViewBacked (master...coins_contract_tests) https://github.com/bitcoin/bitcoin/pull/15137
< emilengler>
Hope it's ok to ask this question here, because I need it for Bitcoin Core development. How can I sign a commit which include squashed commits. Everytime I squash commits the "parent" commit is unsigned
< gwillen>
emilengler: when you squash commits, you destroy the old commits that you squashed, and signatures on them are no good
< gwillen>
you would then need to sign the squashed commit
< gwillen>
perhaps you could elaborate on what you're trying to do