💬 amitiuttarwar commented on pull request "test: create deterministic addrman in the functional tests":
(https://github.com/bitcoin/bitcoin/pull/29007#discussion_r1430822958)
I like the approach of using `-test` to group test-only options

two suggestions for improvement:
* include the options in the help text (eg. how `-onlynet` or `-debug` handles)
* throw some sort of error (or at least warning?) if an invalid option is passed through

💬 kevkevinpal commented on pull request "mempool / rpc: followup to getprioritisedtransactions and delete a mapDeltas entry when delta==0":
(https://github.com/bitcoin/bitcoin/pull/28885#discussion_r1430841261)
@jonatack said that I should run the script in this [comment](https://github.com/bitcoin/bitcoin/pull/28885#discussion_r1406322235) and it lead to there being a indentation, I can remove if needed

🤔 stratospher reviewed a pull request: "net: Make AddrFetch connections to fixed seeds"
(https://github.com/bitcoin/bitcoin/pull/26114#pullrequestreview-1788195228)
ACK dfd635b.

i think this is a good improvement to have! - better peer diversity compared to what's happening on master where we directly open connections to hardcoded seeds.

tested it using `getaddrmaninfo` RPC
- before fixed seeds were added, new table already had around 1679 addresses from the addrfetch connections
- after fixed seeds were added, new table had 2399 addresses

observed 2 addrfetch connections being successfully opened during the 2 minute delay though.

💬 stratospher commented on pull request "net: Make AddrFetch connections to fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/26114#discussion_r1430982146)
7cc4633: could also update the doc comments to `Load addresses of peers from the DNS and from fixed seeds.`

💬 stratospher commented on pull request "net: Make AddrFetch connections to fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/26114#discussion_r1430984088)
611923e: we could remove this fixed seed logic handled in `ThreadOpenConnections` comment.

💬 maflcko commented on issue "Test case for spending bare multisig?":
(https://github.com/bitcoin/bitcoin/issues/29113#issuecomment-1862310039)
pull request welcome :)

💬 maflcko commented on pull request "sqlite: Disallow writing from multiple `SQLiteBatch`s":
(https://github.com/bitcoin/bitcoin/pull/29112#issuecomment-1862494786)
```
File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/wallet_descriptor.py", line 68, in test_concurrent_writes
assert_equal(cache_records, 10000)
File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/util.py", line 57, in assert_equal
raise AssertionError("not
...

💬 maflcko commented on issue "Discussion: Upgrading to C++20":
(https://github.com/bitcoin/bitcoin/issues/23363#issuecomment-1862522407)
> `ArrayLike` sounds like `ranges::contiguous_range` ? https://en.cppreference.com/w/cpp/ranges/contiguous_range

Not exactly. `contiguous_range` is satisfied for `std::vector<int>`, whose size is runtime variable. My `concept ArrayLike` should be `contiguous_range` *and* checking that the size is known at compile time.

💬 maflcko commented on pull request "Update doc/policy/README.md":
(https://github.com/bitcoin/bitcoin/pull/29095#issuecomment-1862556757)
Are you still working on this, or can this be closed up for grabs?

💬 ajtowns commented on issue "Discussion: Upgrading to C++20":
(https://github.com/bitcoin/bitcoin/issues/23363#issuecomment-1862581792)
https://stackoverflow.com/questions/70482497/detecting-compile-time-constantness-of-range-size ? `std::span<X, N>` seems like it's already pretty much the thing that tells you you've got a contiguous range of exactly N X's to me though...

💬 maflcko commented on issue "Discussion: Upgrading to C++20":
(https://github.com/bitcoin/bitcoin/issues/23363#issuecomment-1862605906)
> https://stackoverflow.com/questions/70482497/detecting-compile-time-constantness-of-range-size ?

Ah nice. So this was fixed in the language, but clang never implemented it.

💬 c0deright commented on issue "Old wallet.dat: Error reading wallet database: keymeta found with unexpected path / All keys read correctly, but transaction data or address metadata may be missing or incorrect":
(https://github.com/bitcoin/bitcoin/issues/29109#issuecomment-1862723349)
Ok, thank you very much

💬 murchandamus commented on pull request "fuzz: coinselection, improve `min_viable_change`/`change_output_size`":
(https://github.com/bitcoin/bitcoin/pull/28372#issuecomment-1862768031)
ACK cd810075eddd8b1a7139559b475b56126f70a93d

Passes all my fuzzing crashes and no new crashes with some light additional fuzzing, code LGTM.

💬 nvk commented on pull request "policy: Enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1862776663)
ACK

The current high fee environment is giving a good example as to why is so important to make this behaviour default.

💬 c0deright commented on issue "Old wallet.dat: Error reading wallet database: keymeta found with unexpected path / All keys read correctly, but transaction data or address metadata may be missing or incorrect":
(https://github.com/bitcoin/bitcoin/issues/29109#issuecomment-1862813350)
I did `dumpwallet` and the file created looks like this:

```
# Wallet dump created by Bitcoin Core v26.0.0
# * Created on 2023-12-19T13:47:56Z
# * Best block at time of backup was 821931 (00000000000000000003241b21de4d2d4d3505c2941fba3cccec82f1dbdbce4b),
# mined on 2023-12-19T13:46:59Z

# extended private masterkey: xprv***********************************************************************************************************

L**************************************************n 197
...

💬 michaelfolkson commented on pull request "policy: Enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1862817775)
Concept ACK. I always saw this as inevitable as it reduces mempool reasoning complexity and the supposed merits of zero conf discussion has been done to death.

💬 hebasto commented on issue "Unnecessary symbols being exported in `libbitcoinconsensus.so`":
(https://github.com/bitcoin/bitcoin/issues/26698#issuecomment-1862877305)
The underlying problem is described in https://gcc.gnu.org/bugzilla/show_bug.cgi?id=36022, and it was not considered as a bug.

I thing that moving code that uses C++ template functions/classes out from the `bitcoinconsensus.{h,cpp}` compilation unit, which were suggested in #24994, is a step in the right direction.

💬 hebasto commented on pull request "build: Build `libbitcoinconsensus` from its own convenience library":
(https://github.com/bitcoin/bitcoin/pull/24994#issuecomment-1862914332)
> Concept NACK. In general, I don't like this approach of refactoring code, and adding libraries, to workaround issues with build tooling, especially when its a problem that only exists for a single platform (let alone the DEBUG build, and ). I think this should just be closed in light of #27146 and the migration towards CMake.

I disagree with this NACK.

@fanquake's push back is mostly focused on the Windows-related issue mentioned in the PR description. However, none alternatives are sug
...

💬 fanquake commented on pull request "build: Build `libbitcoinconsensus` from its own convenience library":
(https://github.com/bitcoin/bitcoin/pull/24994#issuecomment-1862925103)
> his code is not fine.

How does the above indicate a problem with our code? Wouldn't this point to the linker being the issue? Does the same happen with `lld`? If not, then it's a problem with `ld`/`libstdc++`, not our code?

💬 hebasto commented on pull request "build: Build `libbitcoinconsensus` from its own convenience library":
(https://github.com/bitcoin/bitcoin/pull/24994#issuecomment-1862929878)
> Wouldn't this point to the linker being the issue?

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=36022 -- `Status: RESOLVED INVALID`