💬 jonatack commented on pull request "rpc, util: avoid string copies in ParseHash/ParseHex functions":
(https://github.com/bitcoin/bitcoin/pull/28172#discussion_r1281325340)
It looks like passing std::strings by reference to const (as done in `ParseHashStr()` in `core_io.h`) would be much better than passing string literals (for which by-value or by-reference-to-const doesn't matter, as you rightly point out).
<details><summary>test</summary><p>
```cpp
// string-passing.cpp
//
// $ clang++ -std=c++17 string-passing.cpp && ./a.out
// foo(string) took 436096 cycles
// bar(string) took 156324 cycles
// foo("string literal") took 1464669 cycles
// bar("stri
...
(https://github.com/bitcoin/bitcoin/pull/28172#discussion_r1281325340)
It looks like passing std::strings by reference to const (as done in `ParseHashStr()` in `core_io.h`) would be much better than passing string literals (for which by-value or by-reference-to-const doesn't matter, as you rightly point out).
<details><summary>test</summary><p>
```cpp
// string-passing.cpp
//
// $ clang++ -std=c++17 string-passing.cpp && ./a.out
// foo(string) took 436096 cycles
// bar(string) took 156324 cycles
// foo("string literal") took 1464669 cycles
// bar("stri
...
💬 achow101 commented on issue "Berkley Database Errors":
(https://github.com/bitcoin/bitcoin/issues/28197#issuecomment-1661480732)
This error indicates that there is already a copy of that wallet file already loaded. The error message tells you which is the already loaded file. You can load these wallets mutually exclusively (unload the other one before loading) and just compare their contents when loaded. Since they are copies of each other, you should only need one.
(https://github.com/bitcoin/bitcoin/issues/28197#issuecomment-1661480732)
This error indicates that there is already a copy of that wallet file already loaded. The error message tells you which is the already loaded file. You can load these wallets mutually exclusively (unload the other one before loading) and just compare their contents when loaded. Since they are copies of each other, you should only need one.
💬 MarcoFalke commented on pull request "util: Replace std::filesystem with util/fs.h":
(https://github.com/bitcoin/bitcoin/pull/28076#issuecomment-1661511351)
> Can you elaborate on why this is a problem?
It just makes developer UX worse. The linters are already horrible, with some contributors having quit (at least parts of the code base) because of them. If someone pushes code and then has to wait more than 20 minutes (a full tidy CI run on 4 CPUs, 30+ minutes on a dirty ccache) to figure out they'll have to replace `std::to_string` with `ToString`, it is certainly more annoying than having to wait 30 milliseconds:
```
$ time git grep 'std::t
...
(https://github.com/bitcoin/bitcoin/pull/28076#issuecomment-1661511351)
> Can you elaborate on why this is a problem?
It just makes developer UX worse. The linters are already horrible, with some contributors having quit (at least parts of the code base) because of them. If someone pushes code and then has to wait more than 20 minutes (a full tidy CI run on 4 CPUs, 30+ minutes on a dirty ccache) to figure out they'll have to replace `std::to_string` with `ToString`, it is certainly more annoying than having to wait 30 milliseconds:
```
$ time git grep 'std::t
...
🤔 MarcoFalke reviewed a pull request: "wallet: Implement independent BDB parser"
(https://github.com/bitcoin/bitcoin/pull/26606#pullrequestreview-1558219665)
Looks like CI is red?
(https://github.com/bitcoin/bitcoin/pull/26606#pullrequestreview-1558219665)
Looks like CI is red?
💬 MarcoFalke commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1281410852)
style nit: I don't think this method is a subset of our streams. Many streams can't seek to the beginning after a read. It may be better to move this up by 3 lines of code (or more if you want)
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1281410852)
style nit: I don't think this method is a subset of our streams. Many streams can't seek to the beginning after a read. It may be better to move this up by 3 lines of code (or more if you want)
💬 MarcoFalke commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1281412977)
style nit: If you remove this, it may be possible to feed in a "happy" wallet.dat (created with vanilla Bitcoin Core) as a fuzz input. See https://github.com/bitcoin-core/qa-assets/pull/140#issuecomment-1660088315
Though, I haven't tested this.
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1281412977)
style nit: If you remove this, it may be possible to feed in a "happy" wallet.dat (created with vanilla Bitcoin Core) as a fuzz input. See https://github.com/bitcoin-core/qa-assets/pull/140#issuecomment-1660088315
Though, I haven't tested this.
💬 MarcoFalke commented on pull request "kernel: Prune leveldb headers":
(https://github.com/bitcoin/bitcoin/pull/28186#discussion_r1281421341)
style nit: `const DataStream&` seems fine here, but if you want to pass an immutable view of raw bytes, `Span<const std::byte>` may be better. (`Span{ssKey}` should do the conversion, but it will likely happen implicitly by the compiler already).
(https://github.com/bitcoin/bitcoin/pull/28186#discussion_r1281421341)
style nit: `const DataStream&` seems fine here, but if you want to pass an immutable view of raw bytes, `Span<const std::byte>` may be better. (`Span{ssKey}` should do the conversion, but it will likely happen implicitly by the compiler already).
💬 MarcoFalke commented on pull request "kernel: Prune leveldb headers":
(https://github.com/bitcoin/bitcoin/pull/28186#discussion_r1281421788)
Also, `CharCast` can be moved to the cpp file in the last commit, if you want
(https://github.com/bitcoin/bitcoin/pull/28186#discussion_r1281421788)
Also, `CharCast` can be moved to the cpp file in the last commit, if you want
💬 ajtowns commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1281498873)
A `std::array` can be viewed as a specific form of map that uses keys from a range of integers starting at 0 that's known at compile time, with either few or no missing elements. Where that's the case, an array is more efficient (smaller storage, no dynamic allocation needed, faster lookups, better caching behaviour etc) and more convenient (eg, `x = a[3]` works even if `a` is a `const&` which isn't true for a map, requiring a [much more convoluted approach]https://github.com/bitcoin/bitcoin/pul
...
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1281498873)
A `std::array` can be viewed as a specific form of map that uses keys from a range of integers starting at 0 that's known at compile time, with either few or no missing elements. Where that's the case, an array is more efficient (smaller storage, no dynamic allocation needed, faster lookups, better caching behaviour etc) and more convenient (eg, `x = a[3]` works even if `a` is a `const&` which isn't true for a map, requiring a [much more convoluted approach]https://github.com/bitcoin/bitcoin/pul
...
💬 darosior commented on pull request "blockstorage: XOR blocksdir *.dat files":
(https://github.com/bitcoin/bitcoin/pull/28052#issuecomment-1661649303)
Concept ACK.
(https://github.com/bitcoin/bitcoin/pull/28052#issuecomment-1661649303)
Concept ACK.
👋 MarcoFalke's pull request is ready for review: "blockstorage: XOR blocksdir *.dat files"
(https://github.com/bitcoin/bitcoin/pull/28052)
(https://github.com/bitcoin/bitcoin/pull/28052)
💬 MarcoFalke commented on pull request "blockstorage: XOR blocksdir *.dat files":
(https://github.com/bitcoin/bitcoin/pull/28052#discussion_r1281548137)
Done
(https://github.com/bitcoin/bitcoin/pull/28052#discussion_r1281548137)
Done
📝 supernormand opened a pull request: "Create extracoin "
(https://github.com/bitcoin/bitcoin/pull/28198)
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/extracoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
extracoin Core user experience or extracoin Core developer experience
significantly:
* Any test improvements or new tests that
...
(https://github.com/bitcoin/bitcoin/pull/28198)
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/extracoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
extracoin Core user experience or extracoin Core developer experience
significantly:
* Any test improvements or new tests that
...
✅ fanquake closed a pull request: "Create extracoin"
(https://github.com/bitcoin/bitcoin/pull/28198)
(https://github.com/bitcoin/bitcoin/pull/28198)
📝 fanquake locked a pull request: "."
(https://github.com/bitcoin/bitcoin/pull/28198)
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/extracoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
extracoin Core user experience or extracoin Core developer experience
significantly:
* Any test improvements or new tests that
...
(https://github.com/bitcoin/bitcoin/pull/28198)
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/extracoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
extracoin Core user experience or extracoin Core developer experience
significantly:
* Any test improvements or new tests that
...
💬 MarcoFalke commented on pull request "blockstorage: XOR blocksdir *.dat files":
(https://github.com/bitcoin/bitcoin/pull/28052#issuecomment-1661738608)
Rebased, and made every commit a refactor, except for the one that has release notes and test changes :)
(https://github.com/bitcoin/bitcoin/pull/28052#issuecomment-1661738608)
Rebased, and made every commit a refactor, except for the one that has release notes and test changes :)
💬 fanquake commented on pull request "test: dedup file hashing using `sha256sum_file` helper":
(https://github.com/bitcoin/bitcoin/pull/27572#issuecomment-1661842143)
Concept ACK. Did you want to address the review comment, or just mark as resolved.
(https://github.com/bitcoin/bitcoin/pull/27572#issuecomment-1661842143)
Concept ACK. Did you want to address the review comment, or just mark as resolved.
💬 MarcoFalke commented on pull request "lint: fix custom mypy cache dir setting":
(https://github.com/bitcoin/bitcoin/pull/28184#discussion_r1281660991)
If it is not needed, it may be better to remove it.
(https://github.com/bitcoin/bitcoin/pull/28184#discussion_r1281660991)
If it is not needed, it may be better to remove it.
💬 theStack commented on pull request "test: dedup file hashing using `sha256sum_file` helper":
(https://github.com/bitcoin/bitcoin/pull/27572#discussion_r1281680165)
Thanks for reviewing! Decided to keep the name as it is, as sha256 falls into the SHA message digests category (see e.g. https://linux.die.net/man/1/shasum) and for the sake of the test concrete type of hash used doesn't matter anyway. All we care is detecting if the wallet file contents have been changed.
(https://github.com/bitcoin/bitcoin/pull/27572#discussion_r1281680165)
Thanks for reviewing! Decided to keep the name as it is, as sha256 falls into the SHA message digests category (see e.g. https://linux.die.net/man/1/shasum) and for the sake of the test concrete type of hash used doesn't matter anyway. All we care is detecting if the wallet file contents have been changed.
💬 hebasto commented on issue "ci: Future of macOS and Windows MSVC CI tasks":
(https://github.com/bitcoin/bitcoin/issues/28098#issuecomment-1661920835)
> Thus, someone would have to sponsor an amount of roughly 5kUSD/mo for those two tasks.
According to Cirrus CI stats for June / July 2023, the usage of resources was equivalent to compute credits/USD as follows:
- Windows -- 2275 / 1941
- Linux -- 2546 / 3160
- macOS -- 2053 / 3568
Therefore, future of Linux tasks should be considered as well.
(https://github.com/bitcoin/bitcoin/issues/28098#issuecomment-1661920835)
> Thus, someone would have to sponsor an amount of roughly 5kUSD/mo for those two tasks.
According to Cirrus CI stats for June / July 2023, the usage of resources was equivalent to compute credits/USD as follows:
- Windows -- 2275 / 1941
- Linux -- 2546 / 3160
- macOS -- 2053 / 3568
Therefore, future of Linux tasks should be considered as well.