💬 maflcko commented on pull request "p2p: TxOrphanage revamp cleanups":
(https://github.com/bitcoin/bitcoin/pull/32941#discussion_r2224569048)
> Hmm, my best guess is a compiler/sanitizer bug.

Jup, gcc uses heuristics that are sometimes off, see https://gcc.gnu.org/bugzilla/show_bug.cgi?id=Wstringop-overflow.

If you have access to the affected gcc version and confirm that sipa's patch works, you can push it. If not, it seems fine to leave as-is and let a dev using the affected gcc version submit a workaround, if they want to.

💬 Eunovo commented on pull request "validation: ensure assumevalid is always used during reindex":
(https://github.com/bitcoin/bitcoin/pull/31615#discussion_r2224573243)
Done

💬 Eunovo commented on pull request "validation: ensure assumevalid is always used during reindex":
(https://github.com/bitcoin/bitcoin/pull/31615#discussion_r2224573495)
Done

💬 Eunovo commented on pull request "validation: ensure assumevalid is always used during reindex":
(https://github.com/bitcoin/bitcoin/pull/31615#discussion_r2224573797)
Done

💬 l0rinc commented on pull request "doc: update toc entries in `developer-notes.md`":
(https://github.com/bitcoin/bitcoin/pull/33040#discussion_r2224600530)
Done

💬 maflcko commented on pull request "doc: update toc entries in `developer-notes.md`":
(https://github.com/bitcoin/bitcoin/pull/33040#issuecomment-3106127049)
Seems fine to squash and call it "doc: regenerate toc"?

💬 maflcko commented on pull request "fuzz: add mempool_dag fuzzer for transaction dependency testing":
(https://github.com/bitcoin/bitcoin/pull/33038#issuecomment-3106136753)
@frankomosh Is this code llm generated?

💬 frankomosh commented on pull request "fuzz: add mempool_dag fuzzer for transaction dependency testing":
(https://github.com/bitcoin/bitcoin/pull/33038#issuecomment-3106185275)
> @frankomosh Is this code llm generated?

have had some LLM assistance, especially in structuring and best c++ practices. Is it a problem @maflcko

💬 maflcko commented on pull request "wallet: Set minversion to FEATURE_LATEST during migration":
(https://github.com/bitcoin/bitcoin/pull/33041#issuecomment-3106206164)
> No test as any test requires very old versions of Bitcoin Core.

It should be possible to test this, because only a wallet needs to be generated. I am happy to write one as a follow-up.

lgtm ACK 5aa758861cf1399842fd0bea3a42d5b0cafcb0f6

💬 maflcko commented on pull request "wallet: Set minversion to FEATURE_LATEST during migration":
(https://github.com/bitcoin/bitcoin/pull/33041#issuecomment-3106257919)
Actually, on a second thought, I think it is cleaner to go with just https://github.com/bitcoin/bitcoin/pull/32977, because the only stated reason for this pull request is "consistency", but I think this ship has sailed anyway, because plenty sqlite wallets exists with a different minversion. They'll have to be dealt with anyway and keeping the migrate logic as-is documents that better. But no strong opinion, I'd say anything is fine here.

📝 w0xlt opened a pull request: "[POC] wallet: Enable non-electronic (paper-based) wallet backup with codex32"
(https://github.com/bitcoin/bitcoin/pull/33043)
This PR introduces support for exporting and restoring wallet seeds using the [codex32](https://github.com/BlockstreamResearch/codex32) format, enabling non-electronic (paper-based) wallet backups.

To accomplish this, the patch ports the `codex32.{c,h}` implementation from Core Lightning to C++, integrating it with Bitcoin Core's libraries. Corresponding unit tests for codex32 encoding and decoding are also included.

Because Bitcoin Core wallets currently do not store the seed material by
...

🤔 Eunovo reviewed a pull request: "RPC: Add reserve member function to `UniValue` and use it in `blockToJSON` function"
(https://github.com/bitcoin/bitcoin/pull/31179#pullrequestreview-3046065014)
Re-ACK https://github.com/bitcoin/bitcoin/pull/31179/commits/5d82d92aff7c11ce17ee809c060e37f73a8a687a

💬 maflcko commented on pull request "fuzz: add mempool_dag fuzzer for transaction dependency testing":
(https://github.com/bitcoin/bitcoin/pull/33038#issuecomment-3106338128)
Historically LLM generated pull requests in this repo were easy to dismiss, because they were obviously wrong on the surface level and usually the tests and CI were failing in similarly obvious ways. However, it now seems that non-trivial LLM generated pull request in this repo looked reasonable on a first glance (compiles and passes CI), but they still miss the point (adding tests without increasing test coverage, or adding features that are useless and counter-productive https://github.com/bit
...

🚀 fanquake merged a pull request: "Update secp256k1 subtree to latest master"
(https://github.com/bitcoin/bitcoin/pull/33036)

💬 fanquake commented on pull request "consensus: Remove mainnet checkpoints":
(https://github.com/bitcoin/bitcoin/pull/25725#issuecomment-3106591087)
Was done in #31649.

👍 stickies-v approved a pull request: "rpc, test: Fix JSON parsing errors in unloadwallet and getdescriptoractivity RPCs"
(https://github.com/bitcoin/bitcoin/pull/32845#pullrequestreview-3046329114)
re-ACK c5c1960f9350d6315cadbdc95fface5f85f25806

nit: clang-format doesn't agree with some of the spacing changes, if you force push again could be nice to run it on each commit to ensure consistency

💬 stickies-v commented on pull request "rpc, test: Fix JSON parsing errors in unloadwallet and getdescriptoractivity RPCs":
(https://github.com/bitcoin/bitcoin/pull/32845#discussion_r2224838548)
> It's also consistent with most of the other unit tests under `src/wallet/test`

Fair enough, I wasn't aware.

> and reduces the chance of symbol collisions

I think all of this stuff is meant to have internal linkage, so if reducing symbol collisions is the goal (which seems sensible) an anonymous namespace might make more sense? Potentially with a `using namespace wallet`, even though at the moment we're only using one `wallet` symbol.

Anyway, not important, and following convention
...

✅ frankomosh closed a pull request: "fuzz: add mempool_dag fuzzer for transaction dependency testing"
(https://github.com/bitcoin/bitcoin/pull/33038)

💬 frankomosh commented on pull request "fuzz: add mempool_dag fuzzer for transaction dependency testing":
(https://github.com/bitcoin/bitcoin/pull/33038#issuecomment-3106595570)
> Historically LLM generated pull requests in this repo were easy to dismiss, because they were obviously wrong on the surface level and usually the tests and CI were failing in similarly obvious ways. However, it now seems that non-trivial LLM generated pull request in this repo looked reasonable on a first glance (compiles and passes CI), but they still miss the point (adding tests without increasing test coverage, or adding features that are useless and counter-productive [#32949 (comment)](h
...

✅ fanquake closed an issue: "Enable PCP by default?"
(https://github.com/bitcoin/bitcoin/issues/31663)

🚀 fanquake merged a pull request: "Enable `-natpmp` by default"
(https://github.com/bitcoin/bitcoin/pull/33004)