💬 S3RK commented on pull request "rpc: encryptwallet help, mention HD seed rotation and backup requirement":
(https://github.com/bitcoin/bitcoin/pull/28980#issuecomment-1842392872)
ACK ca09415e630f0f7de9160cab234bd5ba6968ff2d

👍 maflcko approved a pull request: "test: fix v2 transport intermittent test failure (#29002)"
(https://github.com/bitcoin/bitcoin/pull/29006#pullrequestreview-1766943717)
lgtm. Can't hurt to have this helper, because it also makes code less verbose

💬 maflcko commented on pull request "rpc: fix getrawtransaction segfault":
(https://github.com/bitcoin/bitcoin/pull/29003#issuecomment-1842408340)
lgtm test-was-added ACK 9075a446461ccbc446d21af778aac50b604f39b3

👍 MarnixCroes approved a pull request: "rpc: encryptwallet help, mention HD seed rotation and backup requirement"
(https://github.com/bitcoin/bitcoin/pull/28980#pullrequestreview-1766963845)
ack ca09415e630f0f7de9160cab234bd5ba6968ff2d

💬 stratospher commented on pull request "rpc: addpeeraddress tried return error on failure":
(https://github.com/bitcoin/bitcoin/pull/28998#issuecomment-1842434756)
Concept ACK. it's nice to clearly define success/failure paths for `addpeeraddress`. hopefully after #29007, these rare collisions won't happen since a fixed `nKey` can be used.

💬 maflcko commented on pull request "rpc: addpeeraddress tried return error on failure":
(https://github.com/bitcoin/bitcoin/pull/28998#issuecomment-1842440758)
Maybe just do https://github.com/bitcoin/bitcoin/pull/29007 and the close this one?

💬 TheCharlatan commented on pull request "build: Introduce internal kernel library":
(https://github.com/bitcoin/bitcoin/pull/28690#discussion_r1416964693)
Good catch, I think though this file also falls in the category of https://github.com/bitcoin/bitcoin/pull/28690#discussion_r1408086308, and since it does not introduce any further potentially controversial headers, I'd rather drop this change altogether. This can be re-visited once/if we have some accounting of the headers.

💬 willcl-ark commented on pull request "rpc: encryptwallet help, mention HD seed rotation and backup requirement":
(https://github.com/bitcoin/bitcoin/pull/28980#issuecomment-1842507850)
ack ca09415e630f0f7de9160cab234bd5ba6968ff2d

⚠️ badergithub opened an issue: "Hi"
(https://github.com/bitcoin/bitcoin/issues/29008)

✅ willcl-ark closed an issue: "Hi"
(https://github.com/bitcoin/bitcoin/issues/29008)

:lock: fanquake locked an issue: "Hi"
(https://github.com/bitcoin/bitcoin/issues/29008)

💬 willcl-ark commented on pull request "Embedding ASMap files as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#issuecomment-1842552592)
A general question about this approach: do most contemporary OSes handle loading large amounts of (potentially) unused data into physical memory well-enough, that they would (likely) _not_ load this extra 1.5-3MB of data in the case that the `-asmap` is not being used? Perhaps this will be handled by the demand paging system? If not, it seems to be a bit of a shame to increase binary size by 20% in all cases.

It seems to make most sense to me to take the path suggested by @sipa, including the
...

💬 ismaelsadeeq commented on issue "Test `policyestimator_tests/BlockPolicyEstimates` failure":
(https://github.com/bitcoin/bitcoin/issues/29000#issuecomment-1842556217)
Thats because the fee estimator now updates asynchronously, and there are about 200 `removeForBlock`
https://github.com/bitcoin/bitcoin/blob/6d5790956f45e3de5c6c4ee6fda21878b0d1287b/src/test/policyestimator_tests.cpp#L98

We suppose to wait for the fee estimator to catch up right after the loop below line 113 just as @maflcko said.

I will open a pull that will fix this and some outstanding comments from @willcl-ark in #28368

💬 naumenkogs commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#issuecomment-1842572200)
ACK afc29b15e262b4ff402d479ec77ab8507552bcbc

💬 TheCharlatan commented on pull request "build: Introduce internal kernel library":
(https://github.com/bitcoin/bitcoin/pull/28690#issuecomment-1842572276)
Thank you for the review @ryanofsky,

Rebased 5a235048500a38fae691396cb59f6697032b4deb -> 2086d1d4c3f40cce34647995ead4bff22967ffd8 ([kernelInternalLib_10](https://github.com/TheCharlatan/bitcoin/tree/kernelInternalLib_10) -> [kernelInternalLib_11](https://github.com/TheCharlatan/bitcoin/tree/kernelInternalLib_11), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelInternalLib_10..kernelInternalLib_11))

* Addressed @ryanofsky's [comment](https://github.com/bitcoin/bitcoin/pull/
...

💬 0xB10C commented on pull request "rpc: addpeeraddress tried return error on failure":
(https://github.com/bitcoin/bitcoin/pull/28998#discussion_r1417047413)
> Is a copy the right way, or does it need to be removed from the expected tried table?

Just removing the address from the _expected_ tried table does not work. It's still in the new table. So the question would rather be: "Do we need to retry adding the address to the tried table?". I guess that's debatable. We're only running into this case very rarely when there's a collision - all other runs test that this works. This should be solved with #29007.

> Maybe add steps to reproduce the tes
...

💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#issuecomment-1842597688)
@sr-gi fixed the co-authorship. Thank you for patience :)

took @maflcko suggestion on deterministic tests.

💬 maflcko commented on issue "Increase fuzz coverage in the wallet":
(https://github.com/bitcoin/bitcoin/issues/27272#issuecomment-1842602935)
> The wallet has (almost) no coverage: https://maflcko.github.io/b-c-cov/fuzz.coverage/src/wallet/index-sort-l.html. We should change that!

For reference, the current OSS-Fuzz coverage link: https://storage.googleapis.com/oss-fuzz-coverage/bitcoin-core/reports/20231205/linux/src/bitcoin-core/src/wallet/report.html

💬 maflcko commented on pull request "test: create deterministic addrman in the functional tests":
(https://github.com/bitcoin/bitcoin/pull/29007#discussion_r1417082950)
I wonder if this can be generalized to pass test-only options (along with a helper `HasTestOption`, similar to `IsDeprecatedRPCEnabled`?

The user-facing debug-help output of this seems the wrong place to document test-only options, and the source code seems sufficient to be self-documenting.

```suggestion
argsman.AddArg("-test=<option>", "Pass a test-only option", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
```

💬 maflcko commented on pull request "test: create deterministic addrman in the functional tests":
(https://github.com/bitcoin/bitcoin/pull/29007#discussion_r1417083583)
In any case, `(default: %s)", "relay"` is wrong, no?