💬 rkrux commented on pull request "Wallet: Add `max_tx_weight` to transaction funding options (take 2)":
(https://github.com/bitcoin/bitcoin/pull/29523#discussion_r1656664613)
Interesting, I agree with this point. Returning the exact size of the transaction would make the RPC definition more robust.

💬 rkrux commented on pull request "Wallet: Add `max_tx_weight` to transaction funding options (take 2)":
(https://github.com/bitcoin/bitcoin/pull/29523#discussion_r1656669632)
This commit (along with this^ suggestion) [b3ac117](https://github.com/bitcoin/bitcoin/pull/29523/commits/b3ac1179ff90fe261af4e6dc9c7af64e1518b435) can be a separate PR as well, doesn't seem necessary for this PR to be merged if I didn't miss anything.

💬 willcl-ark commented on issue "Add "maxuploadtargettimeframe" to change the timeframe considered by "maxuploadtarget"":
(https://github.com/bitcoin/bitcoin/issues/30176#issuecomment-2194078864)
Would it not be easier here to simply divide the monthly allowance by the number of days in the month, and use this as the `maxuploadtarget`?

Adding a redundant second setting vs just calculating a daily target doesn't make much sense to me. Even if we did add a monthly target (specified in days), then many months have different numbers of days in them, so this new handy setting will be wrong for many of them.

The best solution for you would seem to be to divide your monthly allowance by t
...

✅ maflcko closed an issue: "error cross compiling linux X64 => 32 bit i686"
(https://github.com/bitcoin/bitcoin/issues/30330)

💬 maflcko commented on issue "error cross compiling linux X64 => 32 bit i686":
(https://github.com/bitcoin/bitcoin/issues/30330#issuecomment-2194109016)
Closing for now, because this is not a Bitcoin Core issue, and solutions have been provided anyway.

💬 alfonsoromanz commented on pull request "Assumeutxo: bugfix on loadtxoutset with a divergent chain + tests":
(https://github.com/bitcoin/bitcoin/pull/29996#issuecomment-2194112241)
Rearranging the order of the commits to make it easier to test the bugfix: the first test (53f714d8bfd2331651445bcadb773a10f4d30264) can be run without the bugfix and is expected to fail, but it should succeed after applying the fix (65343ec49a6b73c4197dfc38e1c2f433b0a3838a).

I also updated the PR title and description to reflect that this PR is not a test-only PR anymore

💬 willcl-ark commented on pull request "init: Add option for rpccookie permissions (replace 26088)":
(https://github.com/bitcoin/bitcoin/pull/28167#discussion_r1656768153)
Moved in 9eff5601059

💬 willcl-ark commented on pull request "init: Add option for rpccookie permissions (replace 26088)":
(https://github.com/bitcoin/bitcoin/pull/28167#discussion_r1656768252)
Added in 49d5bfdd7e7

💬 willcl-ark commented on pull request "init: Add option for rpccookie permissions (replace 26088)":
(https://github.com/bitcoin/bitcoin/pull/28167#discussion_r1656768303)
Thanks, updated in 49d5bfdd7e7

💬 willcl-ark commented on pull request "init: Add option for rpccookie permissions (replace 26088)":
(https://github.com/bitcoin/bitcoin/pull/28167#discussion_r1656768365)
Oh no! fixed in 49d5bfdd7e7

💬 willcl-ark commented on pull request "init: Add option for rpccookie permissions (replace 26088)":
(https://github.com/bitcoin/bitcoin/pull/28167#discussion_r1656768428)
I agree on reflection, gave them new names in 49d5bfdd7e7

💬 willcl-ark commented on pull request "init: Add option for rpccookie permissions (replace 26088)":
(https://github.com/bitcoin/bitcoin/pull/28167#discussion_r1656768473)
I've un-removed the comment in 9eff5601059, as I agree with @ryanofsky that not changing the existant behaviour is the "safer" default, and new behaviour can be introduced with the new option.

👍 rkrux approved a pull request: "test: add coverage for `node` field of `getaddednodeinfo` RPC"
(https://github.com/bitcoin/bitcoin/pull/30339#pullrequestreview-2144759617)
tACK [e38eadb](https://github.com/bitcoin/bitcoin/pull/30339/commits/e38eadb2c2d93d2ee3c9accb649b2de144b3732e)

`make, test/functional` are successful.
Thanks for testing the filtering by node functionality, and replacing the comments with info/debug logs as suggested by @tdb3.

💬 alfonsoromanz commented on pull request "assumeutxo: Check snapshot base block is not in invalid chain":
(https://github.com/bitcoin/bitcoin/pull/30267#issuecomment-2194199411)
Re-ACK 2f9bde69f45c7a9fdcf0c65f9e1305391a6f1f28. The RPC code looks much cleaner after the refactor. Also, it seems very useful to get the error message in the RPC response rather than having to rely on the logs in some scenarios if you are an RPC user.

🤔 alfonsoromanz reviewed a pull request: "test: Added coverage to Block not found error using gettxoutsetinfo"
(https://github.com/bitcoin/bitcoin/pull/30340#pullrequestreview-2144886875)
tACK 6809ffd8a6c589c515af48da2dd8367e4c6c2c26. I ran `test/functional/feature_coinstatsindex.py`, and the test passes.

+1 to informing the log as @tdb3 suggests.

You can now update your PR description to remove the note about the function reordering and the node restart.

💬 maflcko commented on pull request "WIP: Simplify SipHash":
(https://github.com/bitcoin/bitcoin/pull/30317#discussion_r1656887109)
Yeah, seems fine to remove the recursion, given that real code probably doesn't evaluate the hash recursively.

Unrelated, I don't think `uint256` is generally guaranteed to be `uint64_t`-aligned. While probably not a problem here, the cast won't work in the general case:

```
bench/crypto_hash.cpp: runtime error: store to misaligned address 0x7ffe64f6caa1 for type 'uint64_t' (aka 'unsigned long'), which requires 8 byte alignment
0x7ffe64f6caa1: note: pointer points here
00 00 00 00 00
...

💬 willcl-ark commented on issue ""netinfo" doesn't show IPv6 "Local addresses"":
(https://github.com/bitcoin/bitcoin/issues/30165#issuecomment-2194540124)
Presumably in your setup your reverse SSH is connecting via ipv4, therefore bitcoind will detect it has an "active" (localhost) ipv4 connection and decide that its external ip address must be the one you gave it via the `- externalip=143.22.123.17` option.

OP reads to me that you want to have Node (A) bitcoind advertising both ipv4 and (an) ipv6 address(es) which are both reachable on Node (B), so that other nodes can connect to your node (A) via ipv4 or ipv6 on Node (B). Is that correct?


...

📝 ryanofsky opened a pull request: "logging: Use LogFatal instead LogError for fatal errors"
(https://github.com/bitcoin/bitcoin/pull/30347)
Use `LogFatal` instead `LogError` for fatal errors.

Keep using `LogError` for nonfatal errors.

Also rename `LogWarning` to `LogCritical` to be clear it is only intended to be used for "severe problems that the node admin should address," as described in the documentation.

This PR is a draft so other ideas and approaches can be discussed.

⚠️ ryanofsky opened an issue: "RFC: Misused LogError and LogWarning macros"
(https://github.com/bitcoin/bitcoin/issues/30348)
As a programmer, when I see `LogError` and `LogWarning` macros, I assume `LogError` should be used to log information about a failure that happened, and `LogWarning` should be used to log information about an unusual condition which happened that might indicate a real problem depending on other factors and intent.

But [developer notes](https://github.com/bitcoin/bitcoin/blob/b27afb7fb774809c9c075d56e44657e0b607a00c/doc/developer-notes.md) suggest very different meanings for these macros. They
...

💬 brunoerg commented on pull request "test: fix debug log assertion in p2p_i2p_ports test":
(https://github.com/bitcoin/bitcoin/pull/30345#issuecomment-2194572754)
Concept ACK