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

💬 mzumsande commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1462341834)
line should be removed now that `supports_v2_p2p` is a property, it would probably throw an AttributeError if it was hit (which it isn't)

📝 stickies-v opened a pull request: "rpc: improve submitpackage documentation and other improvements"
(https://github.com/bitcoin/bitcoin/pull/29292)
`submitpackage` requires the package to be topologically sorted with the child being the last element in the array, but this is not documented in the RPC method or the error messages.

Also sneaking in some other minor improvements that I found while going through the code:
- Informing the user that `package` needs to be an array of length between `1` and `MAX_PACKAGE_COUNT` is confusing when `IsChildWithPackage()` requires that the package size >= 2. Remove this check to avoid code duplicati
...

💬 Roasbeef commented on pull request "Add test for negative transaction version w/ CSV to tx_valid.json":
(https://github.com/bitcoin/bitcoin/pull/29291#issuecomment-1904769620)
> ACK. Somewhat surprisingly, decoderawtransaction is showing a positive version number:

From 2019 onwards, bitcoind started to display transactions versions as unsigned everywhere: https://github.com/bitcoin/bitcoin/pull/16525

This was prompted by a `rust-bitcoin` issue pointing out they were using a `u32` instead of a `i32` for their transaction struct.

💬 ryanofsky commented on issue "assumeutxo: nTx and nChainTx violations in CheckBlockIndex":
(https://github.com/bitcoin/bitcoin/issues/29261#issuecomment-1904776805)
> Do we not consider regtest "testing"?

The comment is vague, but it's really referring to unit tests.

When I suggested adding the two conditions allowing `pindex->nChainTx == 0` "for testing" in https://github.com/bitcoin/bitcoin/pull/27596#discussion_r1323226418, the reason was to avoid crashes in unit tests, because some unit tests skipped calling `ReceivedBlockTransactions` and therefore skipped setting a `nChainTx` value:

https://github.com/bitcoin/bitcoin/blob/651fb034d85eb5db561b
...

💬 brunoerg commented on pull request "rpc: improve submitpackage documentation and other improvements":
(https://github.com/bitcoin/bitcoin/pull/29292#discussion_r1462427700)
```suggestion
"The package must solely consist of a child and its parents. None of the parents may depend on each other.\n"
```

👋 LarryRuane's pull request is ready for review: "test: test_bitcoin: allow -testdatadir=<datadir>"
(https://github.com/bitcoin/bitcoin/pull/26564)

💬 murchandamus 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#discussion_r1462348934)
In "Implement ImprovesFeerateDiagram" (9257f33d259bf68f1df1cc41b7b3caaea341782f):
Nit: The documentation is in opposite order as the checks. Perhaps switch these two lines.

💬 murchandamus 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#discussion_r1462350586)
In "Implement ImprovesFeerateDiagram" (9257f33d259bf68f1df1cc41b7b3caaea341782f):

```suggestion
return strprintf("%s has both ancestor and descendant, exceeding cluster limit of 2", txid_string);
```