< fanquake>
Have queried GitHub in regards to CODEOWNERS and it's write permissions requirement. re #20200. https://0bin.net/paste/ZYpiRX9U#s67wu+CndcC5mYMooLlnvzqN5SrKW8119sQOKLa+Ju3
< bitcoin-git>
bitcoin/master 6954156 Hennadii Stepanov: qt: Fix visual quality of text in QR image
< bitcoin-git>
bitcoin/master 49984b4 Jonas Schnelli: Merge bitcoin-core/gui#71: Fix visual quality of text in QR image
< jonasschnelli>
I'm again banned in #bitcoin-dev and #bitcoin and #bitcoin-wizards ... if anyone know how to unban: thanks.
< jtimon>
is activation being discussed somewhere else beyond the thread "Modern Soft Fork Activation" ?
< jtimon>
or is the discussion stuck? I'll re-read, but I remember feeling like my points weren't being addressed
< fanquake>
There is ##taproot-activation if you aren't in there already
< jtimon>
I asked questions that I'm pretty sure were ignored
< jtimon>
fanquake: awesome, thanks
< bitcoin-git>
[bitcoin] bitcoin-foundation-admin opened pull request #20229: Adding a new Bitcoin logo, and release it under the public domain license. (master...master) https://github.com/bitcoin/bitcoin/pull/20229
< bitcoin-git>
[bitcoin] MarcoFalke closed pull request #20229: Adding a new Bitcoin logo, and release it under the public domain license. (master...master) https://github.com/bitcoin/bitcoin/pull/20229
< jonasschnelli>
Lol #20229
< gribble>
https://github.com/bitcoin/bitcoin/issues/20229 | Adding a new Bitcoin logo, and release it under the public domain license. by bitcoin-foundation-admin · Pull Request #20229 · bitcoin/bitcoin · GitHub
< jonasschnelli>
On the other hand... they are right about the metadata in my original SVG
< jonasschnelli>
MarcoFalke: not sure if removing the description text helps much... now they certainly claim censorship
< queip>
that's the joke there? png version looks normal
< queip>
btw lol at what ever SVG editor: rgb(96.862745%, that's really precise RGB :o
< jonasschnelli>
I guess my SVG is bad because I think I used Adobe Illustrator to create it and haven't cleand it.
< jonasschnelli>
MarcoFalke: good point about the website... probably good you have removed that text
< jonasschnelli>
The SV people recently started foundation activity in Switzerland.... I think its them. The webpage also links to bitcoin.com.
< MarcoFalke>
The website asks for donations. Obviously don't send anything to their address
< MarcoFalke>
I thought we blocked them already last time they spammed
< MarcoFalke>
Maybe it's a new account
< fanquake>
there's always a new account
< queip>
MarcoFalke: probably that troll, now asks in #bitcoin with some fake DMCA I guess
< MarcoFalke>
What a time sink. Best to ignore
< * queip>
thumbsup. recently some fake-DMCA abusers got counter sued and lost money, just to keep in mind
< kinlo>
heh, it has been quite a while since I had someone using all the insults he can think of in my private windows :)
< queip>
I am curious now, they commited some bitcoin.svg just to change git glame to them or something, got rejected, and now are DMCA-mad that their pressious bitcoin.svg sits there in PR? or smth? (besides that svg file is 404 anyway already)
< kinlo>
do they really open dmca takedowns @ github?
< queip>
kinlo: if yes then great opportunity to ditch github. Or at least prepare to do so in future. if we distribute PRs, Issues, and maybe searching all servers for git remote modules, then we can ignore any such problems. anyway if bitcoin GH is officially mirrored to other gits, wouldn't hurt to promote that knowledge in places
< kinlo>
queip: surely github has experience with children trying to annoy their dmca department
< kinlo>
given the talk he gave me in private, I don't think he's very mature
< jonasschnelli>
For the one not following the GUI repository: https://github.com/bitcoin-core/gui/pull/108 (interactive mempool statistics). Ideas, reviews and testing welcome
< jonasschnelli>
join #bitcoin-core-gui
< jonasschnelli>
##bitcoin-core-gui
< bitcoin-git>
[bitcoin] hebasto opened pull request #20230: wallet: Fix bug when just created encrypted wallet cannot get address (master...201023-signal) https://github.com/bitcoin/bitcoin/pull/20230
< dongcarl>
What's the reason that enabling fuzz disables all other targets
< sipa>
what else would it do?
< dongcarl>
I feel like I'm missing something cuz I expect it to just add targets instead of affecting other targets?
< sipa>
dongcarl: ah, it needs a special compile flag (-fsanitize=fuz), which is incompatible with having a main function
< sipa>
it's not --enable-fuzz itself that's incomaptible with it (that's just switching the targets), but because it needs a compiler flag that's mutually exclusive with normal code
< dongcarl>
Haha I see!
< sipa>
dongcarl: there is an issue somewhere to instead enable building the fuzz test *code* (but without fuzzing) even in normal build mode
< sipa>
which would greatly improve my grievances about it, that it's so easy to break compilation of the fuzz tests when you're changing other code, and won't notice because it needs a special build
< sipa>
however the fuzz tests started using C++17 a while ago, so we need to wait until the main code is built with that too
< dongcarl>
sipa: Totally agree... That's exactly what happened to me...
< jonatack>
there seems to be agreement to have a dedicated fee_rate option that uses a fixed unit of sat/vB
< jonatack>
to replace overloading conf_target and estimate_mode
< jonatack>
and it's not too hard to do
< jonatack>
mostly test-writing to be sure all the plumbing is working
< sipa>
so what would the changes be w.r.t 0.20 ?
< meshcollider>
Concept ACK on fee_rate
< jonatack>
either as-is with 11413 merged and fixups in 20220, e.g. overloading
< luke-jr>
I don't understand the proposed solution
< jonatack>
or stop overloading them asap before release
< jonatack>
before we have to do a deprecation cycle
< jonatack>
to change them
< luke-jr>
overloading them isn't a problem, so long as it's properly documented?
< jonatack>
luke-jr: it's fraught
< sipa>
jonatack: sorry for all the questions, i haven't read all these issues... but i'd like to understand what the problem is and what is being changed
< achow101>
sipa: I don't think explicit feerate is in 0.20?
< jonasschnelli>
oh. Why do we overload "conf_target" with a feerate.. :/
< luke-jr>
jonasschnelli: it's only for positional
< luke-jr>
jonasschnelli: with named args, they have separate names
< sipa>
mostly to make sure we're not doing anything that breaks compatibility
< jonatack>
achow101: no. it was merged late june
< luke-jr>
jonasschnelli: for positional, it's pretty reasonable
< sipa>
jonatack: would the feerate argument to `settxfee` be affected?
< jonatack>
luke-jr: it's mixing types, and look at the fundraw and createpsbt helps...
< luke-jr>
jonatack: so the bug is documentation only
< jonatack>
sipa: so far settxfee is not changed by 11413
< sipa>
or existing RPCs that have feerates in their response?
< jonatack>
luke-jr: i use it for positional, and it's scary. a bit more reassuring with -named
< jonatack>
(have used it many times, i still re-check every time)
< jonatack>
sipa: so far i've looked at input params/options, not yet at output
< sipa>
if we're introducing a sat/vB option, wouldn't it be better to have it everywhere?
< jonatack>
i think so, yes. there seems to be fairly strong agreement to migrate to it.
< sipa>
otherwise people could be copying output from estimatesmartfee and have it be interpreted as a different unit
< sipa>
"migrate" ?
< sipa>
you can't break compatibility
< jonatack>
so we have to stay with BTC/kB?
< luke-jr>
isn't that the reason we use BTC at all in RPC?
< sipa>
for existing RPCs, definitely
< sipa>
but you could add extra output arguments, and extra input arguments
< luke-jr>
otherwise it'd be better to have satoshis everywhere..
< sipa>
feerate, feerate_satvb e.g.
< luke-jr>
sipa: that breaks positional
< sipa>
how so?
< luke-jr>
sipa: positional doesn't have an arg name
< sipa>
it'd be a completely separate argument
< luke-jr>
that's terribly ugly
< sipa>
yes
< jonatack>
luke-jr: so far, in the rpcs where i've added feerate, i placed it just before verbose, which isn't dangerous
< jonatack>
or could be after verbose as well, either way
< jonatack>
e.g. last
< sipa>
jonatack: so verbose moved?
< jonatack>
sipa: no, this is what i'm looking at rn
< jonatack>
adding "fee_rate" or "feerate_sat_vb" to sendtoaddress/sendmany/send etc
< luke-jr>
btw why use vB at all, if not for API compatibility? ;)
< luke-jr>
maybe it should be sat/WU ;)
< sipa>
i think sat/vB makes sense, it's the most common unit used in practice for fees
< sipa>
at least as an option
< sipa>
but compatibility is a problem
< sipa>
previous times when new units in RPC was brought up (mostly in the context of using sat instead of BTC for absolute amounts), it was suggested that this'd be done through a completely new version of the RPC API
< sipa>
though that of course easily scope-creeps into discussions about what else to change
< jonatack>
luke-jr: if it was me doing it greenfield, i might use sat/kvB (and call it sat/sipa for better marketing)
< jonatack>
sipa: hm
< sipa>
goh please no
< luke-jr>
lol
< meshcollider>
ugh, rpc versioning
< jonatack>
for the 3 send rpcs, there is currently no feerate-like param
< luke-jr>
can we split this topic up into two: what is actually _broken_ right now? 2) what is a future backward compat burden?
< jonatack>
bumpfee, fundraw and WCFB do have them
< emzy>
As long as minimum fee is 1 sat/vB, this unit makes sense.
< luke-jr>
emzy: JSON-RPC does not treat integers as special
< sipa>
many client libraries do, though
< sipa>
but indeed, JSON only has a "number" type with no distinction between integers and floating-point
< jonatack>
luke-jr: minimum fixes are in #20220, except maybe additional clarity in the help about the confusing edges, which there currently are
< jonatack>
what MarcoFalke, wumpus, Murch, kallewoof and I have been discussing is not overloading conf_target and estimate_mode before it's too late and released
< luke-jr>
sipa: I can't think of a sane way to prepare for broken client libs combined with sub-sat/WU fee rates :p
< michaelfolkson>
What is WU?
< meshcollider>
Weight unit
< luke-jr>
weight units; ie, what consensus is really using
< michaelfolkson>
Ah ta
< jonatack>
Questions:
< luke-jr>
jonatack: what is the difference between `int target = value.get_int();` and `const int target{value.get_int()};` that makes it so essential?
< jonatack>
- move forward with fix to not overload those two params?
< luke-jr>
I think they should be overloaded
< jonatack>
- call it feerate or fee_rate?
< jonatack>
luke-jr: it's WIP, no need to discuss style nits yet
< Murch>
luke-jr: Do you think floats of sat/vB would be a problem?
< sipa>
it is very late to still make changes to RPC arguments at this point
< luke-jr>
Murch: JSON-RPC doesn't have floats, everything is precise decimal
< luke-jr>
Murch: even if some clients might need to used floats for it, I don't see it likely to be a practical issue
< luke-jr>
after all, floats have plenty of precision?
< Murch>
So how do people input BTC/kB right now?
< jonatack>
ISTM it can be simpler to add a fixed-unit feerate than keep overloading, and simpler code as well
< jonatack>
above all, better for users
< luke-jr>
jonatack: ?
< jonatack>
and i've been dogfooding the current version quite a bit
< luke-jr>
callers should be able to specify fee as an estimate mode or absolute feerate.. I see no case where overloading is weird
< sipa>
Murch: 0.00123456 if you want 123.456 sat/vB
< jonatack>
Has anyone actually used it?
< jonatack>
The explicit feerate with the overloaded args.
< Murch>
sipa: Is that then not a float?
< sipa>
Murch: JSON doesn't have a concept of floats/integers, just "numbers"
< jonatack>
It's awfully odd to send txns with a feerate set with conf_target=2 and estimate_mode="sat/vb"
< sipa>
jonatack: but can't you use fee_rate=2 as well?
< luke-jr>
jonatack: 20220 is long, and starting from the top looks like a bunch of completely unnecessary changes; I'm not opposed to them, but it's not helpful to understand what is in need of fixing
< Murch>
I guess I'm missing the distinction, but I understand that it's not a problem to put in something with a value between 0 and 1
< jonatack>
the names do not correspond at all to what is being done
< sipa>
Murch: read the JSON spec, it just has a number type
< sipa>
Murch: the problem is with client libraries, which may map the number type to floating point types, which is bad for currency reasons
< luke-jr>
Murch: JSON-RPC numbers are decimal strings; floats are fixed-size approximations
< sipa>
bitcoin core has no problem with this, it parses the decimals exactly from json, without conversion to a floating point type at any point
< Murch>
luke-jr: Thanks, I see.
< sipa>
i think it's a good idea to try to avoid numbers with a decimal in our RPC, but that's not worth breaking compatibility over
< jonatack>
luke-jr: with an important fix that i noted with a review comment and some help documentation fixes
< jonatack>
(some of the helps are currently wrong)
< Murch>
My alternative proposal was sat/kvB, which gives us an additional 3 decimal of precision for numbers that are safe integers
< Murch>
(which, though would also get parsed as floats in client libraries potentially, I guess)
< emzy>
Murch: thats also msat/vB
< Murch>
emzy: sure.
< sipa>
Murch: the bitcoin core RPC also lets you pass any amount as a string, in case your client library can't produce JSON numbers without going through a floating-point type
< luke-jr>
emzy: but msat isn't an existing unit (in Bitcoin/Core at least)
< emzy>
luke-jr: but Lighting uses it already.
< sipa>
so you can have integer sats internally in the application, and format them as "%i.%06i" % (sats // 1000000, sats % 1000000) for example (python like)
< luke-jr>
emzy: unfortunately :p
< sipa>
we'll add a command line option to rename msat/vB to sat/kvB for luke-jr
< sipa>
;)
< Murch>
mh, my nickname is the only one with a capital letter. Are you all anti-capitalists?
< emzy>
I don't like the 1/k I think it is confusing
< luke-jr>
lol
< jonatack>
I'm not sure what people would like to do here. What we have now does have some issues per #19543 and other issues I found while adding tests, but happy to not work on it further if it's not needed.
< Murch>
presumably `conf_target` and `feerate` would be exclusive or one would supersede if both are provided (exclusive is cleaner, though)
< jonatack>
Ok, in issue 19543 I thought there was a pretty clear direction, but it seems not, and I suspect no one has been dogfooding the current new feature.
< luke-jr>
Murch: ideally, if both were provided, we'd error
< luke-jr>
or rather, if the wrong one were provided
< luke-jr>
estimate_mode (perhaps should be renamed fee_mode) specifies which one is correct
< jonatack>
luke-jr: we cannot rename estimate_mode
< luke-jr>
(and with "fee_mode" there, "fee_rate" seems obvious over "feerate")
< luke-jr>
jonatack: why not?
< jonatack>
it's used for the fee estimation since some time
< luke-jr>
so?
< luke-jr>
we support multiple names
< luke-jr>
deprecate the old one and keep allowing it
< jonatack>
This is a mess.
< jonatack>
I thought we had a good solution, but it seems best for me to drop it.
< jonatack>
I'm done.
< sipa>
:(
< jonatack>
luke-jr: I with you had chimed in on the discussion in 19543
< jonatack>
we seemed in agreement I thought
< jonatack>
I'll get back to catching up on the reviewing.
< achow101>
any other topics for the last 5 minutes?
< michaelfolkson>
Is that an effective Approach NACK luke-jr?
< luke-jr>
michaelfolkson: I don't understand his proposed approach still :/
< sipa>
i worry that i may have contributed to jonatack's frustration here by commenting without understanding the problem well
< michaelfolkson>
Ok well if you both do look over it it would be good to get a formal Approach NACK if you really don't like it.
< michaelfolkson>
I haven't understood it all either
< jonatack>
sipa, no worries, I'm not frustrated, it's just clearly too early in that people haven't tripped on the issues yet.
< jonatack>
achow101: did you want to do high priority?
< Murch>
How come that parameters to RPC are defined separately instead of in a dictionary that applies to all uses of the same parameter?
< Murch>
I.e. why is something like feerate not defined once for the whole codebase?
< achow101>
jonatack: I think all the high priority is already listed in the milestone
< jonatack>
achow101: that's true
< sipa>
Murch: what do you mean with "defined once" ?
< sipa>
code for parsing it?
< achow101>
#endmeeting
< lightningbot>
Meeting ended Fri Oct 23 20:00:27 2020 UTC. Information about MeetBot at http://wiki.debian.org/MeetBot . (v 0.1.4)
< Murch>
like a class that provides the parameter parsing, logic, and sanitation that can be used by various rpc to
< sipa>
we have that for amounts (AmountFromValue), and historically, feerates were always just amounts (per k(v)B)
< michaelfolkson>
I have a question for achow101 if he's sticking around. You said in your blog post it will be possible to migrate legacy wallets to descriptor wallets. How does the descriptor wallet obtain the descriptor from the legacy wallet?
< sipa>
jonatack: i think it'd be useful if you added a summary of the actual RPC changes in 20220 ?
< sipa>
s/?//
< jtimon>
sipa: is there not a CFeeRate class anymore?
< sipa>
there is, but it doesn't RPC argument parsing
< sipa>
feerates were just constructed from an amount
< jtimon>
oh, I see
< achow101>
michaelfolkson: going through all the keys in the legacy wallet and creating descriptors for them. it requires considering everything that IsMine matches on
< jtimon>
yeah, yeah, from an amount is the one I know, murch wants one to parse from rpc
< jtimon>
got it
< achow101>
for HD wallets, it's pretty simple with computing the xprv for the hd seed and using a single descriptor. for non-HD but still only key things, it's a descriptor for each key
< achow101>
for wallets with watchonly things and multisigs, it gets complicated
< jtimon>
so what's the status on using bitcoin core with hardware wallets, multisig and all those cool things? is it still a separated branch?
< jtimon>
or it is merged now?
< luke-jr>
not even close afaik
< jtimon>
:(
< sipa>
PSBT works
< jonatack>
sipa: do you mean discuss in the 20220 PR description the RPC changes of 11413?
< achow101>
jtimon: if you're willing to do some command line stuff, it works
< achow101>
*do command line stuff and use hwi separately
< sipa>
jonatack: no, what changes in 20220
< jtimon>
but I need to get a different branch with scripts and stuff, no?
< achow101>
jtimon: no
< michaelfolkson>
achow101: It effectively has to ask the legacy wallet what the equivalent descriptor is. I'd have thought in some scenarios this would be hard as the legacy wallet wasn't set up to answer that question.
< jtimon>
achow101: oh, I see, so I guess it's more or less a while back, but now merged, nice
< sipa>
jtimon: you need HWI or other software that can talk to the hardware wallet
< jonatack>
sipa, 20220 just adds missing test coverage, fixes a bug to make bumpfee work again, and does doc updates that were left over from 11413 (which was 3 years old when it was merged)
< jtimon>
are there any plans to add that stuff to the qt interface?
< sipa>
jonatack: oh ok, that explains why i couldn't find anything
< achow101>
michaelfolkson: it's just kind of complex. but the set of scriptPubKeys is finite and not log(n!) so it's doable
< sipa>
achow101, michaelfolkson: i think it depends on what level of compatibility you're talking about; constructing a set of descriptors that match exactly what a legacy wallet *right now* considers IsMine() is doable i think
< jonatack>
yes, 20220 doesn't change the RPCs. I started changing them in 20231 today...simpler UI and code.
< jtimon>
achow101: oh, wow, and the gui is separated? I missed a lot
< sipa>
making it also treat IsMine() any future script the legacy would have considered... is probably impossible if you want to cover al edge cases
< sipa>
jtimon: same project, different repo
< sipa>
it's still compiled as one thing
< sipa>
so it's just the development/review workflow that moved elsewhere
< jtimon>
oh, ok, just 2 projects for the PRs and stuff, right?
< achow101>
yeah
< achow101>
sipa: as long as we don't change legacy ismine, we don't have to worry about future stuff
< jtimon>
well, still very cool that this is getting into the gui
< sipa>
achow101: it's also only crazy edge cases that you can't cover i think
< sipa>
achow101: like having a HD chain that has a future key X, where multi(2,X,Y,Z) is a watch-only script, with Y and Z private keys you already have
< achow101>
sipa: well stop using the legacy wallet after it migrates :p
< sipa>
achow101: of course
< sipa>
just means you can't silently do the conversion
< luke-jr>
"Welcome to Bitcoin Core 2022.11. You are using an ancient wallet version, that will not be supported in the next release. Do you want to upgrade now? This will invalidate old backups!"
< achow101>
luke-jr: that's probably what will happen anyways
< achow101>
actually, the current migration pr doesn't let you use the legacy stuff anyways. it currently makes a new descriptor wallets and essentially imports the descriptors for the legacy wallet
< sipa>
achow101: how big is an sqlite wallet.dat if it has 6000 imported descriptors?
< sipa>
oh, or will it convert hdchains correctly and only have 6?
< achow101>
it should correctly convert hd chains
< achow101>
I haven't tested the migration pr with sqlite yet. I think it might be broken
< sipa>
jonatack: so is there anything you'l were thinking of trying to get in 0.21, besides 20220?
< jonatack>
sipa: re-reading, i now see your question "but can't you use fee_rate=2 as well"
< jonatack>
and that's part of the oddness, with explicit feerate feature, no
< jonatack>
and only for BTC/vB iiuc, feeRate doesn't handle sat/B but conf_target with estimate_mode does handle it
< sipa>
too many ideas floating around
< jonatack>
yeah it's fairly unsimple and unintuitive
< jonatack>
tagged for 0.21 i also have #20115 and #20120, but no hurry, still a week :)
< bitcoin-git>
[bitcoin] jnewbery opened pull request #20233: addrman: make sanity checks a runtime option (master...2020-10-addrman-sanity) https://github.com/bitcoin/bitcoin/pull/20233
< achow101>
if a multisig isn't being watched, should it be migrated during descriptor wallet migration?
< sipa>
achow101: as in, it's not IsMine() in the legacy wallet?
< achow101>
sipa: yes
< sipa>
i'd say no
< achow101>
but it was added and we can sign for it
< sipa>
can you have a concept of an "unwatched" descriptor?
< achow101>
nope
< sipa>
which helps for signing, but isn't watching anything?
< sipa>
it would be strange
< achow101>
we can sign if it's part of a psbt
< achow101>
but we wouldn't be able to fill that psbt with the script
< sipa>
right
< sipa>
descriptor wallets really don't have a concept that corresponds to that
< achow101>
indeed
< sipa>
and i think that's a good thing
< sipa>
but it may be useful to have an option at migration time to convert all solvable things in the legacy wallet to watched things in the descriptor one
< sipa>
if you really want, you'd be able to convert a legacy wallet into a descriptor wallet for balance watching, and another one with "all cruft i can participate in signing for"