💬 maflcko commented on pull request "lint: Check for missing bitcoin-config.h includes":
(https://github.com/bitcoin/bitcoin/pull/29408#issuecomment-1934074211)
The idea to `grep` was taken from theuni (https://github.com/bitcoin/bitcoin/issues/26972#issuecomment-1930911069)

✅ maflcko closed an issue: "doc: List identifiers used from header in #if(def)"
(https://github.com/bitcoin/bitcoin/issues/26972)

💬 maflcko commented on issue "doc: List identifiers used from header in #if(def)":
(https://github.com/bitcoin/bitcoin/issues/26972#issuecomment-1934080395)
> I used a few combinations of git grep -L -E <tokens> to find all files which don't require bitcoin-config.h, and used that in a git grep -l bitcoin-config.h -- <those files> to find the files that don't require the include.

Iwyu should be able to find includes that are present, but not needed. So ideally, your result should match what iwyu finds.

While iwyu can find some *missing* includes, it can't find *all*. So I've written a lint check in https://github.com/bitcoin/bitcoin/pull/29408
...

💬 brunoerg commented on pull request "p2p: Allow whitelisting manual connections":
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1482969906)
Yes, we can call `Init` without setting all values in `Options`. I will change it to use `DEFAULT_WHITELISTFORCERELAY` and `DEFAULT_WHITELISTRELAY`.

💬 daniel-btc-nym commented on issue "getJsonToken assumes underlying string is null-terminated but requires end pointer":
(https://github.com/bitcoin/bitcoin/issues/28260#issuecomment-1934115970)
I still need to look into how MSAN is integrated into the current unit test suite. Questions like: how can I run the unit tests I added under MSAN? Are all unit tests run under MSAN with a certain make target ... the basics. I am very new to this repository here :)

💬 brunoerg commented on pull request "p2p: Allow whitelisting manual connections":
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1482974024)
Nice catch! I'll address it.

💬 maflcko commented on issue "getJsonToken assumes underlying string is null-terminated but requires end pointer":
(https://github.com/bitcoin/bitcoin/issues/28260#issuecomment-1934121806)
It is probably not worth it to try to setup msan, as it requires building llvm from scratch.

It is easier to just use valgrind. For example: `valgrind ./src/test/test_bitcoin -t vector_tests`

💬 ismaelsadeeq commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1482976877)
Fixed

💬 ismaelsadeeq commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1482977350)
Updated 👍🏾

💬 ismaelsadeeq commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1482978519)
Added a test for this thanks.

🤔 jo-elimu requested changes to a pull request: "test: fix RPC coverage check"
(https://github.com/bitcoin/bitcoin/pull/29387#pullrequestreview-1870231673)
@BrandonOdiwuor There is a CI [error](https://github.com/bitcoin/bitcoin/pull/29387/checks?check_run_id=21316696148):
```
Uncovered RPC commands:
- abortrescan
```

💬 ismaelsadeeq commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1482979637)
Fixed

💬 ismaelsadeeq commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1482980736)
Thanks updated the tests.

💬 ismaelsadeeq commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1482981545)
Done

💬 ismaelsadeeq commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1482981736)
Fixed

💬 ismaelsadeeq commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#issuecomment-1934137354)
Thanks for your review @josibake
I fixed all review comments
Forced pushed from a5d10367cf832497af2ac72f8c2c42dd25398c63 to 1fae882f98205afb05e96432ce57212226bc0909 [Compare changes](https://github.com/bitcoin/bitcoin/compare/a5d10367cf832497af2ac72f8c2c42dd25398c63..1fae882f98205afb05e96432ce57212226bc0909)

💬 BrandonOdiwuor commented on pull request "test: fix RPC coverage check":
(https://github.com/bitcoin/bitcoin/pull/29387#issuecomment-1934152480)
@jo-elimu that's the error the fix was to catch. Check the issue

💬 brunoerg commented on pull request "p2p: Allow whitelisting manual connections":
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1482999788)
I think that was a result of many changes during the path. In fact, we do not need `Initialize` anymore. We can concentrate the logic into `AddWhitelistPermissionFlags`.

💬 maflcko commented on pull request "test: fix RPC coverage check":
(https://github.com/bitcoin/bitcoin/pull/29387#issuecomment-1934168380)
In any case, a pull request with a failing CI can not be merged. Someone will need to add coverage for the RPC, here, or in a different pull.

📝 ryanofsky opened a pull request: "multiprocess: Add capnp wrapper for Chain interface"
(https://github.com/bitcoin/bitcoin/pull/29409)
**This is based on #28929.** The non-base commits are:

- [`486d39da5a3e` Add capnp serialization code for bitcoin types](https://github.com/bitcoin/bitcoin/pull/29409/commits/486d39da5a3e96791ad4033a863b606f9ae1ebc6)
- [`25e2ef26bede` Add capnp wrapper for Handler interface](https://github.com/bitcoin/bitcoin/pull/29409/commits/25e2ef26bede691ddf24f426b27987b7917437ba)
- [`22e05493c4f6` Add capnp wrapper for Chain interface](https://github.com/bitcoin/bitcoin/pull/29409/commits/22e05493c4f6
...