💬 josibake commented on pull request "wallet: skip BnB when SFFO is enabled":
(https://github.com/bitcoin/bitcoin/pull/28994#discussion_r1416717975)
My concern is less with the specific wording and more that we are logging low-level algorithm internals that at best are not actionable for the node runner and at worst confusing.

For developers, the algorithm and waste metric (along with additional data points) are already captured in `TRACE5`:

https://github.com/bitcoin/bitcoin/blob/efb54a2bd284639722da794650d2afda66fb0770/src/wallet/spend.cpp#L1128

So my questions are:

1. How is the introduction of this logline related to this _sp
...

🤔 stratospher reviewed a pull request: "test: fix v2 transport intermittent test failure (#29002)"
(https://github.com/bitcoin/bitcoin/pull/29006#pullrequestreview-1766740168)
ACK 00e0658.

cross checked node0's debug log to verify that [this getpeerinfo](https://github.com/bitcoin/bitcoin/blob/6d5790956f45e3de5c6c4ee6fda21878b0d1287b/test/functional/p2p_v2_transport.py#L154) gets called before disconnection is complete.


```
2023-12-05T13:26:56.774275Z [net] [net.cpp:1106] [ProcessReceivedKeyBytes] [net] V2 transport error: V1 peer with wrong MessageStart 0a03cf40
2023-12-05T13:26:56.857216Z [net] [net.cpp:554] [CloseSocketDisconnect] [net] disconnectin
...

💬 stratospher commented on pull request "test: fix v2 transport intermittent test failure (#29002)":
(https://github.com/bitcoin/bitcoin/pull/29006#discussion_r1416731520)
00e0658: wondering if we need to wait for previous disconnections here since the unrelated disconnections happen only with peers where we directly open the sockets to the node right? (tests above use TestNode) i like how the approach is consistent with the rest of the test though.

💬 maflcko commented on pull request "test: fix v2 transport intermittent test failure (#29002)":
(https://github.com/bitcoin/bitcoin/pull/29006#discussion_r1416885349)
nit: unrelated: Would be good to wait for CNode desctruction after disconnection by checking the getpeerinfo RPC. This is what I meant in https://github.com/bitcoin/bitcoin/issues/29002#issuecomment-1841702298

💬 maflcko commented on pull request "test: fix v2 transport intermittent test failure (#29002)":
(https://github.com/bitcoin/bitcoin/pull/29006#discussion_r1416891272)
nit: unrelated: Could have a `self.wait_until` to avoid having to pass the timeout factor every time?

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