💬 Daniel600 commented on pull request "policy: Enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1661182837)
Your research is not thorough and reaches an incorrect conclusion.
As stated many times - we service payment processors and some merchants directly  - Coinspaid services multiple merchants and process a significant amount of BTC they are a well known and active in the space - as I provided back in December 2022 a email from Max the CEO of Coinspaid confirming their use of 0-conf as well as providing there cluster addresses to validate there deposit flows see here again - https://lists.linuxfoun
...

💬 ryanofsky commented on pull request "refactor: Add util::Result failure values, multiple error and warning messages":
(https://github.com/bitcoin/bitcoin/pull/25665#issuecomment-1661192251)
Updated 1de05ef9190202f04f8cbe7746a47cbd66ab540c -> 08f5febfc571220043436bbec96a326beebdee22 ([`pr/bresult2.36`](https://github.com/ryanofsky/bitcoin/commits/pr/bresult2.36) -> [`pr/bresult2.37`](https://github.com/ryanofsky/bitcoin/commits/pr/bresult2.37), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/bresult2.36..pr/bresult2.37)) replacing `util::Messages` with `util::MoveMessages` to work around clang-tidy error `bugprone-use-after-move` (https://cirrus-ci.com/task/665702225123737
...

💬 jonatack commented on issue "Intermittent CI failure "fee too high" in wallet_send.py --descriptors ":
(https://github.com/bitcoin/bitcoin/issues/25164#issuecomment-1661246977)
Seen in `wallet_send.py --legacy-wallet`

https://cirrus-ci.com/task/6115877712560128?logs=ci#L3429

```
node0 2023-08-01T23:17:59.093565Z [httpworker.1] [rpc/request.cpp:181] [parse] [rpc] ThreadRPCServer method=testmempoolaccept user=__cookie__
test 2023-08-01T23:17:59.094000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-p
...

💬 petertodd commented on pull request "policy: Enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1661327327)
On Tue, Aug 01, 2023 at 03:28:31PM -0700, Daniel Lipshitz wrote:
> Your research is not thorough and reaches an incorrect conclusion.
> As stated many times - we service payment processors and some merchants directly  - Coinspaid services multiple merchants and process a significant amount of BTC they are a well known and active in the space - as I provided back in December 2022 a email from Max the CEO of Coinspaid confirming their use of 0-conf as well as providing there cluster addresses to v
...

⚠️ Bitnee opened an issue: "Berkley Database Errors"
(https://github.com/bitcoin/bitcoin/issues/28197)
### Issues, reports or feature requests related to the GUI should be opened directly on the GUI repo

- [X] I still think this issue should be opened here

### Report

Attempting Restoration from 2016 year Wallet

Two Errors (many duplicate wallets supposedly)

BerleyDatabase: Can't open database {} (duplicates fileid 9770020000001b00539d761d0000000000000000 from {.dat}
&
PRUNED NODES {internally solved}

any support is held in much appreciations

💬 ariard commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1281295986)
From my understanding we have `submitpackage` which is the current interface under which *child-with-unconfirmed-parents* can be submitted. The definition is “a topologically sorted package that consists of exactly one child and all of its unconfirmed parents (no other transactions may be present)”.

The definition given by `AncestorPackage` class is “A potential BIP331 Ancestor Package, i.e one transaction with its set of ancestors”, which sounds similar.

If *child-with-unconfirmed-paren
...

💬 ariard commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1281312757)
I think you thought about it in term of `AncestorPackage` API design, though what is the purpose of `SkipWithDescendants` as the “dangle” state of a transaction could be determined as soon as all the transactions are received and `IsPackageWellFormed` ?

Therefore checking than we have available UTXOs in mempool (or in-package) to spend could be done before to call to `AncestorPackage` ctor and `visit` to obtain the `ancestor_subset`, sounds it could be a small perf again assuming `AcceptPacka
...

💬 ariard commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1281317275)
The name could be more verbose to reflect the notion of “connection” it lays on, e.g `m_is_ancestor_package`.

💬 ariard commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1281318967)
Unclear if “(Modified) fees” means `GetModifiedFee()`

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