💬 ariard commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1281323288)
> So you can assume we never construct the miniminer with more than 25 transactions.

Thanks, yes I think `MAX_PACKAGE_COUNT` as enforced by `IsPackageWellFormed` is the upper bound that one can assume in the calculation of `MiniMiner`. This can ease the DoS reasoning to link the max `ancpkginfo` issued at the p2p level with `MAX_PACKAGE_COUNT` as requested by the mempool logic to save on bandwidth both by the local node (the list of wtxids announced) and its peer (`pkgtnxs`’s txn), I think.

💬 ChrisCho-H commented on pull request "script: throw disabled err for op_ver and its variants":
(https://github.com/bitcoin/bitcoin/pull/28169#discussion_r1281323514)
cool, I've implemented!

💬 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
...

💬 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.

💬 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
...

🤔 MarcoFalke reviewed a pull request: "wallet: Implement independent BDB parser"
(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)

💬 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.

💬 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).

💬 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

💬 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
...

💬 darosior commented on pull request "blockstorage: XOR blocksdir *.dat files":
(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)

💬 MarcoFalke commented on pull request "blockstorage: XOR blocksdir *.dat files":
(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
...

✅ fanquake closed a pull request: "Create extracoin"
(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
...

💬 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 :)

💬 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.

💬 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.