✅ 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?

💬 maflcko commented on pull request "test: create deterministic addrman in the functional tests":
(https://github.com/bitcoin/bitcoin/pull/29007#discussion_r1417084172)
```suggestion
const auto options = args.GetArgs("-addrtest");
```

💬 0xB10C commented on pull request "rpc: addpeeraddress tried return error on failure":
(https://github.com/bitcoin/bitcoin/pull/28998#issuecomment-1842629300)
Maybe get #29007 in first. This allows us to remove the getrawaddrman-test retry logic (possibly do it there). Makes the `getrawaddrman` test simpler.

I've addressed the minor comments here, but I think it makes sense to draft this PR for now until #29007 is in. With #29007 we can test the collision case when `addpeeraddress tried` returns `{"success": false, "error": "failed-adding-to-tried"}` as discussed in https://github.com/bitcoin/bitcoin/pull/28998#discussion_r1417047413.

📝 0xB10C converted_to_draft a pull request: "rpc: addpeeraddress tried return error on failure"
(https://github.com/bitcoin/bitcoin/pull/28998)
When trying to add an address to the IP address manager tried table, it's first added to the new table and then moved to the tried table. Previously, adding a conflicting address to the address manager's tried table with test-only `addpeeraddress tried=true` RPC would return `{ "success": true }`. However, the address would not be added to the tried table, but would remain in the new table. This caused, e.g., issue #28964.

This is fixed by new returning `{ "success": false, "error": "..." }`
...

💬 fanquake commented on pull request "[POC] C++20 `std::endian`":
(https://github.com/bitcoin/bitcoin/pull/28674#issuecomment-1842632814)
> Yes that is correct. Also the compiler installed is g++-11.

It would be useful to have compiler/version used/C{XX} flags/configure options (even if just mentioning it's the default) displayed next to the benchmarking information, as I assume this will always be one of the first questions asked in relation to benchmark output.

🤔 maflcko reviewed a pull request: "fuzz: wallet, add target for Spend"
(https://github.com/bitcoin/bitcoin/pull/28236#pullrequestreview-1767252938)
Spend.cpp coverage is not too bad already: https://maflcko.github.io/b-c-cov/fuzz.coverage/src/wallet/spend.cpp.gcov.html

It may be good to clarify the new coverage a bit.

💬 maflcko commented on pull request "fuzz: wallet, add target for Spend":
(https://github.com/bitcoin/bitcoin/pull/28236#discussion_r1417093434)
Seems like this should be a separate fuzz target?

💬 maflcko commented on pull request "fuzz: wallet, add target for `Crypter`":
(https://github.com/bitcoin/bitcoin/pull/28074#discussion_r1417099754)
Could also move them out of the loop, like the others?

👍 maflcko approved a pull request: "fuzz: wallet, add target for `Crypter`"
(https://github.com/bitcoin/bitcoin/pull/28074#pullrequestreview-1767263157)
lgtm

💬 fanquake commented on pull request "build: Enable -Wunreachable-code":
(https://github.com/bitcoin/bitcoin/pull/28999#issuecomment-1842648355)
Concept ACK