< bitcoin-git>
[bitcoin] sipa opened pull request #19230: [TESTS] Move base58 to own module to break circular dependency (master...202006_addr_base58) https://github.com/bitcoin/bitcoin/pull/19230
< sipa>
qt/bitcoin.cpp: In function ‘int GuiMain(int, char**)’:
< sipa>
qt/bitcoin.cpp:460:35: error: ‘Untranslated’ was not declared in this scope
< sipa>
node->initError(strprintf(Untranslated("Error parsing command line arguments: %s\n"), error));
< sipa>
^~~~~~~~~~~~
< fanquake>
Looks like #19176 was merged even though broken for one of the builds
< bitcoin-git>
bitcoin/master 948f113 fanquake: gui: add missing translation.h include to fix build
< bitcoin-git>
bitcoin/master 20e9531 fanquake: Merge #19231: gui: add missing translation.h include to fix build
< bitcoin-git>
[bitcoin] fanquake merged pull request #19231: gui: add missing translation.h include to fix build (master...translations_gui_fixup) https://github.com/bitcoin/bitcoin/pull/19231
< bitcoin-git>
[bitcoin] hebasto opened pull request #19233: Make SetMiscWarning() accept bilingual_str argument (master...200610-bi-warn) https://github.com/bitcoin/bitcoin/pull/19233
< shesek>
0.20.0 does not appear to report descriptor information in getaddressinfo. is this a known bug? or am I missing something? here's a simple reproduction script (basically a simple `importmulti` followed by a `getaddressinfo`) and the output I'm seeing for 0.19.1 and 0.20.0: https://gist.github.com/shesek/cecfe7f154f7a0356d8882f694713e20
< shesek>
oh wait, I'm using pk() and treating it as a base58 address, which it isn't
< harding>
Yeah, I think the question is why bitcoin-cli -regtest deriveaddresses 'pk(0279be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798)#gn28ywm7' returns a P2PKH address.
< shesek>
is that for 0.19, 0.20 or both?
< harding>
Both 0.19.0.1 and 0.20.0 (just tested).
< shesek>
the missing descriptor info was indeed due to using pk() and then requesting the base58 address, it does work as expected with pkh()
< shesek>
harding, yes, it definitely shouldn't do that :)
< shesek>
so what changed between 0.19 and 0.20? it seems like something in 0.20 no longer considers pk() to be p2pkh-like-base58-encodeable
< shesek>
not treating p2pk as base58 encodable in esplora is probably the #1 cause for complaints I'm hearing from people, especially around times when old stashes move around and everyone wants to look them up on the explorers. its hard enough to explain that other block explorers are wrong, but bitcoind doing it too makes this quite more challenging :p (in some other places too)
< shesek>
harding, oddly enough, it seems like importing a pk() descriptor with a label on 0.20, then calling getaddressesbylabel with that label, does return the p2pkh address. and the label does show up in `getaddressinfo` for the p2pkh address, while the `ismine`/`iswatchonly` fields are set to `false`
< shesek>
importing with a label and using getaddressesbylabel is actually how I found myself using the p2pkh address format of a p2pk script in the first place
< wumpus>
fanquake: thanks for fixing the build after #19176, it's kind of strange, pretty sure the PR passed all the checks
< bitcoin-git>
[bitcoin] Sjors closed pull request #13818: More intuitive GUI settings behavior when -proxy is set (master...2018/07/gui-proxy) https://github.com/bitcoin/bitcoin/pull/13818
< provoostenator>
Review nag for #18030; one of those situations where just documenting the confusion seems faster than fixing it :-)
< shesek>
is there a customary notation for referring to the nth script of a descriptor identified by its checksum? something like <checksum>/<index>, akin to <bip32-fingerprint>/<index>?
< wumpus>
provoostenator: nice, thanks for writing that up
< jonatack>
thanks provoostenator
< bitcoin-git>
[bitcoin] hebasto opened pull request #19238: refactor: Replace RecursiveMutex with Mutex in CAddrMan (master...200610-addrman-mx) https://github.com/bitcoin/bitcoin/pull/19238
< Kiminuo>
wumpus, Hi, I have been investigating std::filesystem in c++17 a bit (see https://github.com/bitcoin/bitcoin/pull/19183#issuecomment-641321318) and I would like to lay the groundwork for a worthy PR. I'm just wondering whether you would be willing to answer a few questions along the way - given that you know the old issues with boost::filesystem.
< bitcoin-git>
[bitcoin] jnewbery opened pull request #19239: tests: move generate_wif_key to wallet_util.py (master...2020-06-generate-wif-key) https://github.com/bitcoin/bitcoin/pull/19239
< wumpus>
Kiminuo: sure! though, I'm not sure I'm aware of all the old issues with boost::filesystem, just that there's a long history of them
< wumpus>
(and as said I'm most worried about windows support, it tends to be that the mingw c++ library support lags beind unix/linux — but if it passes all the unit and functional tests even on that platform it's a good indication at least )
< Kiminuo>
wumpus, well, I have two projects in MSVS, one with boost and one with c++17 and so far I have problems with unicode in boost project but c++17 & unicode works well for me - I use fs::u8path (https://en.cppreference.com/w/cpp/filesystem/path/u8path) and not fs::path. This makes me believe that c++17 may actually help with unicode issues. No guarantees at this point though. It's just it may not be that crazy idea to change fs::path -> fs:u8path.
< Kiminuo>
But it's very hard for me to assess whether it's a good idea or not and if it sounds good whether there are corner cases (different platforms, different Bitcoin features,...)
< sipsorcery>
Kiminuo: If you need I can check/help with changes to the file path logic on Windows.
< sipsorcery>
iirc there were a few PR's to fix some Windows unicode issues a year or so ago.
< Kiminuo>
sipsorcery, great! I'll try to prepare a draft PR in coming days and it will be great to collaborate on that then :-)
< Kiminuo>
sipsorcery, if you know about any of that issue, could you send a link? It would be helpful to compile a list of those issues to make review easier
< sipa>
Kiminuo: i briefly looked into replacing fs with something std::filesystem based, but didn't get very far
< sipa>
i did notice that it'd require a separate unique_path
< sipsorcery>
It'd be great to get rid of the Win32 CreateDirectoryA/CreateDirectoryW etc calls and replace with std library calls.
< sipsorcery>
I'll dig up the PR's.
< Kiminuo>
sipa, Yes, it's true about the unique_path, unfortunately. But given a random generator, one can generate a random file name, right?
< achow101>
i have a possibly very dumb idea: hack sqlite's btree module to use the bdb file format
< sipa>
why...?
< achow101>
to keep backwards compatibility and avoid all of the issues with refactoring existing bdb handling code
< achow101>
i also haven't thought this through at all
< sipa>
it sounds like combining the worst of all worlds :)
< sipa>
bdb dependency, complexity of sqlite, and still have a new file format that's not compatible with old software
< achow101>
I think it would be compatible with old software and still remove the bdb dependency
< luke-jr>
O.o
< achow101>
the only reason I thought of this was because I found sqlightning which is sqlite but using lmdb as the btree module
< luke-jr>
so you mean reimplementing bdb?
< achow101>
only the file format
< luke-jr>
also, bdb has no concept of tables/etc
< luke-jr>
2) sqlite doesn't accept outside patches, 3) it would take years for distros to release this version
< sipa>
if you seriously want to implement bdb's file format, please revive logdb instead
< achow101>
the idea was to just be able to toss out the existing bdb handling code without breaking existing compatibility
< sipa>
i'm very confused by what you're suggesting then
< achow101>
basically use sqlite but have it read and write bdb format files. so new wallets can still be opened by old software and old wallets opened by new
< achow101>
so we can use sqlite's acid guarantees and easier implementation but still remain compatible
< achow101>
and not have to maintain the separate stuff to deal with bdb
< achow101>
that was the half baked idea
< luke-jr>
achow101: I don't see how sqlite is easier to implement.. or what problems we have from bdb
< luke-jr>
it seems to be working fine
< achow101>
luke-jr: in the refactor to prepare for other db systems, there's a few questions about whether that has introduced regressions because it's difficult to work out the exact flushing and closing behavior
< sipa>
achow101: i'm baffled how you think introducing sqlite would not break compatibility
< achow101>
sipa: because we would be modifying it to write data in the same way bdb does
< luke-jr>
achow101: okay, but the fact is right now we have years of no problems
< achow101>
luke-jr: no?
< sipa>
achow101: then what's the point?
< sipa>
that seems like a huge task for no gain
< luke-jr>
achow101: ?
< achow101>
sipa: then old software would still be able to read those files
< sipa>
achow101: i must be missing something
< achow101>
sipa: sqlite does a bunch of other things unrelated to the storage format to guarantee consistency
< achow101>
so we would be able to take advantage of those
< achow101>
and just have a different file format
< sipa>
it seems the cost/benefit of such an effort are about 100x worse than staying with bdb, or switching to logdb
< achow101>
it seemed to me to be easier to implement/copy a file format implementation than to logic my way through when and how to flush and close bdb for the refactor
< luke-jr>
achow101: what problems do you think need solving?
< sipa>
making (a) a new backend to sqlite and (b) making and keeping it compatible with bdb eaxh individually seem like far more work (in implementation+testing/qa combined) then just creating a dumb key/value store from scratch
< sipa>
the advantage of sqlite is getting a well-tested robust format and implementation for free; if it needs large customization, i don't see why we'd even pick it in the first place
< luke-jr>
sipa: bdb4.8 is currently/already well-tested and robust in our usage though
< luke-jr>
only point in switching to sqlite IMO is if we want to use the relational features
< achow101>
sipa: to take advantage of how they do atomic commits, do locking and concurrency, etc. the things which are independent of the file format itself.
< sipa>
i don't comprehend how that can seem worth it
< achow101>
for example, sqlite flushes to the database file after every write. this behavior is independent of the database file. because it does this, we could remove the need for PeriodicFlush. We would remove the need for WalletDatabase::Flush entirely
< sipa>
ok?
< sipa>
and that's worth a huge engineering effort to develop and maintain a new backend to sqlite?
< sipa>
to avoid a wrapper in our logic?
< luke-jr>
one advantage of bdb is since it isn't maintained, what we learn about its inner workings *won't* change :p
< achow101>
depends on how huge the engineering effort is
< achow101>
we have a lot of very confusing logic in our code related to flushing
< luke-jr>
achow101: we *could* just flush on every change to bdb
< luke-jr>
there might be reasons we don't, but if so, then switching to sqlite would have the same problems
< achow101>
and the flushing logic is the cause of a lot of confusion in the refactor
< sipa>
i'm sure we'll get through that
< achow101>
luke-jr: we do "flush", but it flushes to the transaction log, not the database file
< wumpus>
I like using sqlite, to be honest, I don't think rolling a new database just for bitcoin core's database is a good idea, yes a dumb store would be enough for key storage, but for transactions it's likely that some more advanced query functionality would be useful at some point
< sipa>
wumpus: i just sqlite because it's so well tested
< sipa>
*like
< achow101>
and getting it to flush into the database file is, apparently, a major task and pain in the ass
< achow101>
but trivial for sqlite
< wumpus>
it's also very well tested
< wumpus>
agree
< luke-jr>
sipa: but so is db4.8 specifically in our use
< sipa>
and maintained
< wumpus>
achow101: yes, the flush has never really been a flush but 'database consolidation'
< wumpus>
(which happens to also do a flush, but that's not the point, the point was to have things in one file)
< luke-jr>
if you want things in one file, look at what backupwallet does
< bitcoin-git>
[bitcoin] dongcarl closed pull request #18072: Use `libc++` headers from macOS SDK instead of from clang (master...2020-01-macos-sdk-with-headers) https://github.com/bitcoin/bitcoin/pull/18072