< luke-jr>
gmaxwell: "new"? didn't we merely just remove a long-deprecated way?
< sipa>
luke-jr: to many people it seems new :)
< sipa>
perhaps instead of "It is recommended to use a new receiving address for each transaction" it should say "Use the request payment button in the receive tab to create new addresses."
< meshcollider>
yep and these are good hacktoberfest tag issues :)
< gmaxwell>
luke-jr: "new address" process, not new "address process"
< bitcoin-git>
[bitcoin] ken2812221 opened pull request #14480: refactor: Drop boost::this_thread::interruption_point and boost::thread_interrupted in main thread (master...drop-boost-thread-import) https://github.com/bitcoin/bitcoin/pull/14480
< bitcoin-git>
[bitcoin] MeshCollider opened pull request #14481: Add P2SH-P2WSH support to listunspent RPC (master...201810_listunspent_wsh) https://github.com/bitcoin/bitcoin/pull/14481
< bitcoin-git>
[bitcoin] MeshCollider closed pull request #11708: Add P2SH-P2WSH support to signrawtransaction and listunspent RPC (master...201711_signrawtransaction_wsh) https://github.com/bitcoin/bitcoin/pull/11708
< karelb>
Is there some linter on bitcoin core that looks for line length?
< luke-jr>
karelb: hopefully not, as there is no line length limit? :p
< karelb>
:) ok. Some linters say "ok lines should not be longer than 80 characters" etc
< karelb>
s/some linters/some best practices/
< luke-jr>
in practice, we do have some hard wrapping in the codebase, but IMO it's a bad idea
< sipa>
we have plenty of lines that are longer :)
< luke-jr>
everyone's editor is a different width, and can soft-wrap as needed
< sipa>
and we do have a suggested coding style defined by the clang formatter config in the repo
< sipa>
but it's not really possible to enforce a strict style without making people waste time dealing with every tiny nit
< sipa>
karelb: make sure you follow conventions for variable/class/... names in new code
< meshcollider>
karelb: is this related/the same/similar to what achow101 is working on
< sipa>
(see developer-notes.md)
< meshcollider>
that is, do both of you know what the other is working on to make sure it doesn't overlap
< karelb>
meshcollider: I think it is slightly different. I have saw his repo and I did not really understand it all that much, but it seemed complementary
< meshcollider>
ok, as long as you're aware of it :)
< karelb>
I am, I just don't really understand the code :(
< karelb>
Hm it seems it is actually similar to what I did, but using univalue
< karelb>
sipa: thanks for the link to dev-notes. I will use that
< karelb>
(I did not write any big c++ code in years, I am surprised it works)
< meshcollider>
lol
< * sipa>
learned c++ from the bitcoin codebase
< sipa>
i guess it explains some things...
< karelb>
:D
< karelb>
"Class member variables have a m_ prefix" - I don't see that in many class variables in bitcoin codebase?
< luke-jr>
karelb: old code isn't changed, just new code is expected to follow these
< karelb>
oh ok
< karelb>
so it's not a good idea to look at existing code for reference
< meshcollider>
no there are so many styles everywhere it is very inconsistent
< karelb>
ooooh ok
< meshcollider>
e.g. new class naming convention is to not start the class name with C iirc, but most existing classes start with C like CWallet
< sipa>
karelb: read the first parafraph
< sipa>
*paragraph
< karelb>
Oooooh ok. Hmm, looking in github on src/ history, it's interesting how little is there new classes added and mostly it's fixes of existing code. Well not that surprising actually.
< sipa>
yeah :)
< sipa>
karelb: also, new code is often written in new files
< karelb>
Also - how much does bitcoin (at least new code) use all the const correctness stuff? I never know how to write it correctly and where to add `const`
< bitcoin-git>
[bitcoin] HatboyWonder opened pull request #14484: changed request payment button text and tab description (master...master) https://github.com/bitcoin/bitcoin/pull/14484
< sipa>
karelb: const correctness is easy, never add a const cast
< sipa>
if it compiles, you're good
< sipa>
:)
< sipa>
and there are some small parts of the code that are not const correct, but mostly, yes
< sipa>
(in particular the serialization code does some hairy stuff)
< meshcollider>
sipa: I was thinking about the import descriptor thing internally converting to old structures
< meshcollider>
for ranges, how would that work
< meshcollider>
surely not cover the whole 2^31-1 range or whatever?
< meshcollider>
or is that ok
< meshcollider>
or just not support ranges for now
< sipa>
meshcollider: you'd specify the range along with the import
< sipa>
if you specify a billion, you're importing a billion addresses, and the wallet file will likely catch fire and explode
< meshcollider>
only one wildcard is allowed in the path eh? No ambiguity if only a single number is used to specify the range?
< sipa>
you can have multiple hd paths that end with * in a descriptor
< sipa>
but they're always combined pairwise
< sipa>
or in other words, all *s are replaced with the same number in an expansion
< meshcollider>
ah I see, yep
< meshcollider>
and does a range always start from 0
< meshcollider>
or should it be a start, end pair
< sipa>
that's up to the application
< sipa>
the descriptor is really just a list of addresses
< sipa>
the application chooses to evaluate it at certain positions of the list
< meshcollider>
Maybe just look at the test case rather than the whole diff for a concept ack
< meshcollider>
I'll open a PR
< bitcoin-git>
[bitcoin] DesWurstes opened pull request #14486: Add explicit cast to base58 and bech32 string constants in order to silence GCC warning (master...patch-4) https://github.com/bitcoin/bitcoin/pull/14486
< sipa>
the code in rpcwallet you quote is more recent
< waxwing>
sipa, thx
< sipa>
achow101, meshcollider: looking back at your PRs #14454 and #14019, i'm confused why we need to import the raw pubkey scripts when doing an importmulti
< achow101>
I separated that into 2 PRs because the importing of pubkeys in a p2sh seemed to be orthogonal to the end goal
< sipa>
but why do you need the pubkey itself imported?
< sipa>
i think you just need a mapKeyMetadata entry
< achow101>
so that GetPubKey and/or CreateSig work
< sipa>
GetPubKey should only be needed for PKH schemes
< sipa>
since 14424
< sipa>
i'd really like to avoid making unrelated things watched; for multisig that's even dangerous as someone can trick you into thinking you're being paid by paying to one of the constituent pubkeys rather than the multisig script
< sipa>
oh, CreateSig also takes a CKeyID as input?
< sipa>
oh, no
< sipa>
not anymore
< sipa>
achow101: can you please test if this is still needed since 14424?
< achow101>
with 14424, I don't think that importing pubkeys with p2sh is necessary anymore
< sipa>
okay!
< achow101>
unless we support signing with arbitrary scripts which may contain pubkey hashes
< achow101>
(this has been requested by someone and I said I would look into it)
< sipa>
that's not supported anyway right now
< sipa>
and with descriptors that would become possible without making payments to those pubkeys themselves treated as ismine
< achow101>
eh, the request was actually signing arbitrary scripts in a psbt, so importing pubkeys probably wouldn't be necessary
< achow101>
It could probably be done by bypassing ProduceSignature and doing a simple signer instead for psbts
< sipa>
right
< bitcoin-git>
[bitcoin] achow101 closed pull request #14019: Import pubkeys when importing p2sh with importmulti (master...import-multi-pubkeys) https://github.com/bitcoin/bitcoin/pull/14019
< meshcollider>
sipa: in that case, it looks like I can just delete the entire "// Import public keys." block of code?
< meshcollider>
Because the case of single key addresses P2PK, P2PKH, P2WPKH is already covered above
< meshcollider>
Well, I just need to move the actual import to there
< sipa>
meshcollider: i haven't looked in detail at the implementation
< meshcollider>
I mean, is the only case where we need to import a public key is for a P2PKH/P2WPKH without the private key?
< sipa>
right, and P2SH-P2WPKH
< bitcoin-git>
[bitcoin] ken2812221 opened pull request #14489: refactor: Drop boost::thread and boost::chrono (master...interruptible-thread) https://github.com/bitcoin/bitcoin/pull/14489