💬 ryanofsky commented on pull request "depends: Update libmultiprocess library to fix C++20 macos build error":
(https://github.com/bitcoin/bitcoin/pull/29276#issuecomment-1904414778)
Updated 5dfd24581a7c5497601966a5371d8c33eabcee8a -> b8105b3ed7c97cd6545dba99d0e13c33f183e450 ([`pr/c14.1`](https://github.com/ryanofsky/bitcoin/commits/pr/c14.1) -> [`pr/c14.2`](https://github.com/ryanofsky/bitcoin/commits/pr/c14.2), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/c14.1..pr/c14.2)) just adding a new macos build fix: https://github.com/chaincodelabs/libmultiprocess/pull/93.

I'm pretty sure the new fix is not needed for any current depends builds because depends build
...

💬 brunoerg commented on pull request "fuzz: compare scripts from `Expand` and `ExpandFromCache`":
(https://github.com/bitcoin/bitcoin/pull/28908#issuecomment-1904416619)
Rebased

💬 brunoerg commented on pull request "test: add coverage for bech32m in `wallet_keypool_topup`":
(https://github.com/bitcoin/bitcoin/pull/28928#issuecomment-1904416959)
Rebased

💬 brunoerg commented on pull request "contrib: add test for bucketing with asmap":
(https://github.com/bitcoin/bitcoin/pull/28869#issuecomment-1904418492)
Rebased

💬 vasild commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#issuecomment-1904423201)
Ok, fine. Then, just a comments nit, the recently added comments "Per BIP159, safety buffer" and "BIP159 connections safety window" better be removed? Because that is not "A safety buffer of 144 blocks to handle chain reorganizations"?

💬 hebasto commented on pull request "depends: Do not override `CFLAGS` when building SQLite with `DEBUG=1`":
(https://github.com/bitcoin/bitcoin/pull/29287#discussion_r1462153876)
> Would be better to only use SQLITE_DEBUG?

Thanks! Done.

💬 theStack commented on pull request "rpc: parse legacy pubkeys consistently with specific error messages":
(https://github.com/bitcoin/bitcoin/pull/28336#issuecomment-1904438007)
Rebased on master (to fix CI).

💬 hebasto commented on pull request "Avoid non-self-contained Windows header":
(https://github.com/bitcoin-core/gui/pull/789#issuecomment-1904455841)
@theuni

You might be interested to consider the following example: https://godbolt.org/z/b8rns1e6x

💬 instagibbs commented on pull request "Mempool util: Add RBF diagram checks for single chunks against clusters of size 2":
(https://github.com/bitcoin/bitcoin/pull/29242#issuecomment-1904479665)
> In https://github.com/sipa/bitcoin/commits/pr29242 I pushed another commit which makes CompareFeerateDiagram not modify the diagrams in-place.

Taken only with minor comment changes, and added another test case or two to cover the iterative nature of the check

💬 hebasto commented on pull request "build: always set `-g -O2` in `CORE_CXXFLAGS`":
(https://github.com/bitcoin/bitcoin/pull/29205#issuecomment-1904504426)
While always overriding Autotools' defaults, are there any particular reasons not using [`-O3`](https://gcc.gnu.org/onlinedocs/gcc/Optimize-Options.html#index-O3) instead of `-O2`?

👍 hebasto approved a pull request: "build: always set `-g -O2` in `CORE_CXXFLAGS`"
(https://github.com/bitcoin/bitcoin/pull/29205#pullrequestreview-1837055782)
ACK 00c1e2aa4496b5f038ae5199dbd16d8313766533, I have reviewed the code and it looks OK. Also tested `ci/test/00_setup_env_native_valgrind.sh`.

✅ willcl-ark closed an issue: "berkeley database failed to open database environment"
(https://github.com/bitcoin/bitcoin/issues/29286)

💬 willcl-ark commented on issue "berkeley database failed to open database environment":
(https://github.com/bitcoin/bitcoin/issues/29286#issuecomment-1904555895)
OK great. I'll close this for now then.

Do open a new issue if it happens again and you confirm that the wallet database (directory) is present, but failing to load.

💬 pinheadmz commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1462248344)
I actually had this as `params=[]` a few updates ago but that fails lint (see below) but I'll clean it up with a different work-around:

```
A mutable list or dict seems to be used as default parameter value:

test/functional/interface_rpc.py:41:def expect_http_rpc_status(expected_http_status, expected_rpc_error_code, node, method, params={}):

This is how mutable list and dict default parameter values behave:

>>> def f(i, j=[], k={}):
... j.append(i)
... k[i] = True
...
...

💬 darosior commented on pull request "Add test for negative transaction version w/ CSV to tx_valid.json":
(https://github.com/bitcoin/bitcoin/pull/29291#issuecomment-1904569442)
ACK 97181decf5726aab6c5cd01b3e1964072f2531ff

💬 theStack commented on pull request "wallet, rpc: `FundTransaction` refactor":
(https://github.com/bitcoin/bitcoin/pull/28560#issuecomment-1904606796)
Concept ACK

💬 jamesob commented on pull request "Add test for negative transaction version w/ CSV to tx_valid.json":
(https://github.com/bitcoin/bitcoin/pull/29291#issuecomment-1904681862)
ACK. Somewhat surprisingly, `decoderawtransaction` is showing a positive version number:
```
% ./src/bitcoin-cli decoderawtransaction "0000ffff01000100000000000000000000000000000000000000000000000000000000000000000000030251b2010000000100000000000000000000000000"
{
"txid": "d694a1d916d4ffa0b87dc2ed43fa8fdcf3c4a7d89afd765a8ce55ad6becea753",
"hash": "d694a1d916d4ffa0b87dc2ed43fa8fdcf3c
...

💬 jb55 commented on pull request "tracing: network connection tracepoints":
(https://github.com/bitcoin/bitcoin/pull/25832#issuecomment-1904685588)
On Thu, Jan 18, 2024 at 02:03:35AM -0800, 0xB10C wrote:
>Hm the CI failure of `previous releases, qt5 dev package and depends
>packages, DEBUG` (CI job used to find silent-merge conflicts (?)) four
>days ago seems weird. Didn't see any conflicts when rebasing.
>
>Anyway, rebased. Sorry for invalidating your ACK again, @jb55. Fixed
>the discurage -> discourage misspelling. I thought I did it in an
>earlier push.

sorry I haven't been able to sit down and actually test this yet. The
changes look g
...

💬 willcl-ark commented on issue "assumeutxo: nTx and nChainTx violations in CheckBlockIndex":
(https://github.com/bitcoin/bitcoin/issues/29261#issuecomment-1904709745)
> `$ ./src/qt/bitcoin-qt -datadir=/tmp -regtest`

Do we not consider regtest "testing"?

ISTM that only "test mode" (regtest) will fail the assertion with the lines removed.

💬 pinheadmz commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1462363162)
Ok I understand. I'm taking this suggestion and also using `std::move` a few places in `bitcoin-cli` where potentially large result objects are being passed. I am leaving as-is however when the argument is `NullUniValue`, I think that's correct but please let me know if I should use `std::move` at the other call sites as well.