💬 kristapsk commented on pull request "Update rpcauth.py":
(https://github.com/bitcoin/bitcoin/pull/29433#issuecomment-1955227816)
> Not sure it makes sense to do this. If you're parsing JSON, surely you can do the trivial things inside rpcauth yourself?

Probably also true, my Bitcoin node setup script currently just does something like:

```bash
python3 "$SCRIPT_DIR/rpcauth.py" \
"$bitcoin_rpcuser" "$bitcoin_rpcpassword" | grep rpcauth >> /etc/bitcoin/bitcoin.conf
```

💬 luke-jr commented on pull request "Correct tooltip wording for watch-only wallets":
(https://github.com/bitcoin-core/gui/pull/792#discussion_r1496623669)
Despite the confusing interfaces, watch-only wallets are not based on addresses (or they would never see spends). Prefer to use less confusing language here.

💬 ryanofsky commented on pull request "serialization: Support for multiple parameters":
(https://github.com/bitcoin/bitcoin/pull/28929#discussion_r1496625389)
re: https://github.com/bitcoin/bitcoin/pull/28929#discussion_r1496589417

> The warning doesn't make sense and I think it is a compiler bug.

Actually it is not a compiler bug. It happens because of the lifetimebound annotation and goes away if I change:

```diff
- ParamsStream(SubStream&& substream LIFETIMEBOUND, const Params& params LIFETIMEBOUND) : m_params{params}, m_substream{std::forward<SubStream>(substream)} {}
+ ParamsStream(SubStream&& substream, const Params& params LIFE
...

💬 luke-jr commented on pull request "Keep focus on "Hide" while ModalOverlay is visible":
(https://github.com/bitcoin-core/gui/pull/795#issuecomment-1955232548)
Wouldn't it be better to hide the covered widgets instead?

🤔 ryanofsky reviewed a pull request: "serialization: Support for multiple parameters"
(https://github.com/bitcoin/bitcoin/pull/28929#pullrequestreview-1891730489)
Updated e02ab1577f51a6c9af96b49e29ffaa3918ddae6c -> 33d99bad01637a3454358841b5c1aaf099002afb ([`pr/iparams.5`](https://github.com/ryanofsky/bitcoin/commits/pr/iparams.5) -> [`pr/iparams.6`](https://github.com/ryanofsky/bitcoin/commits/pr/iparams.6), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/iparams.5..pr/iparams.6)) adding a new commit simplifying usage of ParamsStream in net.cpp, and fixing inaccurate lifetimebound annotations

💬 ryanofsky commented on pull request "serialization: Support for multiple parameters":
(https://github.com/bitcoin/bitcoin/pull/28929#discussion_r1496650793)
re: https://github.com/bitcoin/bitcoin/pull/28929#discussion_r1496625389

This should be resolved in latest push, which just drops the LIFETIMEBOUND annotation since it isn't always accurate. The only case where specifying the annotation is actually correct is the case where the argument is an lvalue. And in that case, I think the annotation is probably not useful. The language already doesn't allow passing temporaries as rvalues so there is probably not anything the compiler can helpfully war
...

💬 bstin commented on pull request "Update rpcauth.py":
(https://github.com/bitcoin/bitcoin/pull/29433#issuecomment-1955370336)
The default behavior is the same, so no change unless someone specifically wants json output.

For anyone just outputting "rpcauth=XXXXX" directly into bitcoin.conf, yes the grep solution is fine.

But if, in addition to that, you wanted to extract the *value* of rpcauth, then its more steps. (further, this assumes "=" will always be the delimiter)

By providing a machine-friendly output format that makes such deployments less fragile in the future.

💬 ariard commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1496674625)
for code readability, such variables could be renamed `inbounds_flooding_tx_relay` / `outbounds_flooding_tx_relay`, which is matching the low-fanout flooding strategy used for them.

💬 ariard commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1496695880)
I confirm with current implemented bip331 approach there is currently a `MAX_ORPHAN_TOTAL_SIZE` limit.
You can always get a non-standard parent (e.g an under-dust output) and yet the child be policy valid.
There is currently no sanitization of policy equivalence among a set of parent within `ancpkginfo`.
In the future, you could have reconciliation at the `pkgtxns`-level or at package announcement (`ancpkginfo`).
Ideally both, though that something that can be seen once erlay or bip331 are d
...

💬 ariard commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1496683339)
can add a `LogPrint(BCLog::NET, “Non-signaling reconciliation inbound peers flooding %d Outbound peers flooding %d for debug”);` for debug purpose and observation

📝 kevkevinpal opened a pull request: "test: check_mempool_result negative feerate"
(https://github.com/bitcoin/bitcoin/pull/29459)
Adds test coverage in `mempool_accept.py` to check if a negative `maxfeerate` is input into `check_mempool_result`
Asserts "Amount out of range" error message and `-3` error code

Motivated by this [comment](https://github.com/bitcoin/bitcoin/pull/29434/files#r1491112250)

💬 kevkevinpal commented on pull request "rpc: Fixed signed integer overflow for large feerates":
(https://github.com/bitcoin/bitcoin/pull/29434#discussion_r1496832298)
Created a PR to cover this https://github.com/bitcoin/bitcoin/pull/29459

📝 kevkevinpal opened a pull request: "test: assert rpc error for addnode v2transport not enabled"
(https://github.com/bitcoin/bitcoin/pull/29460)
Added coverage for the `addnode` rpc when v2transport is not enabled,
but is set as true when calling `addnode` rpc.

I ran the following to check if this rpc error message
was covered in the functional tests.
`grep -nr "v2transport requested but not enabled" ./test/functional --binary-files=without-match`

Adds test coverage to this line.
https://github.com/bitcoin/bitcoin/blob/master/src/rpc/net.cpp#L339

🤔 ajtowns reviewed a pull request: "mempool: Log added for dumping mempool transactions to disk"
(https://github.com/bitcoin/bitcoin/pull/29402#pullrequestreview-1892071040)
utACK dbdb7320252fd68415e8b76bad5a2169bd7da241

💬 ajtowns commented on pull request "mempool: Log added for dumping mempool transactions to disk":
(https://github.com/bitcoin/bitcoin/pull/29402#discussion_r1496887607)
Remove "from disk" here, if dropping "disk" mentions elsewhere?

💬 Eunovo commented on pull request "test: Fix SegwitV0SignatureMsg nLockTime signedness":
(https://github.com/bitcoin/bitcoin/pull/29400#issuecomment-1955897276)
Tested ACK https://github.com/bitcoin/bitcoin/pull/29400/commits/fab15723b0518acbb1015e64df47dcac0187e92f
Reproduced the error by setting `nLockTime` to `2147483648`

💬 Eunovo commented on pull request "test: Fix SegwitV0SignatureMsg nLockTime signedness":
(https://github.com/bitcoin/bitcoin/pull/29400#discussion_r1496914400)
> can you add a test that fails before this change, but passes after?

Wouldn't that mean that we added a test that tests the test-framework?

💬 Eunovo commented on pull request "Extend signetchallenge to set target block spacing":
(https://github.com/bitcoin/bitcoin/pull/29365#discussion_r1496917274)
@starius Might be useful to leave comment explaining why the params size is limited this way

💬 Eunovo commented on pull request "Extend signetchallenge to set target block spacing":
(https://github.com/bitcoin/bitcoin/pull/29365#issuecomment-1955903839)
Tested ACK
Tried different `signetchallenge` inputs and got the expected results.

💬 maflcko commented on pull request "test: Fix SegwitV0SignatureMsg nLockTime signedness":
(https://github.com/bitcoin/bitcoin/pull/29400#discussion_r1497068772)
> > can you add a test that fails before this change, but passes after?
>
> Wouldn't that mean that we added a test that tests the test-framework?

Yes, it should be possible to write an internal python unit test to check the behavior of `SegwitV0SignatureMsg`. An alternative would be to extend a python functional test to cover more values of `nLockTime` and compare the `SegwitV0SignatureMsg` behavior against Bitcoin Core via the RPC interface.