💬 pablomartin4btc commented on pull request "rpc, test: Fix JSON parsing errors in unloadwallet and getdescriptoractivity RPCs":
(https://github.com/bitcoin/bitcoin/pull/32845#discussion_r2224162725)
Thanks for pointed that out. Forgot to remove `wallet::` in the `EnsureUniqueWalletName()` call. The reason I wrapped it in `namespace wallet` is that it helps to avoid verbose `wallet::` prefixes and reduces the chance of symbol collisions. It's also consistent with most of the other unit tests under `src/wallet/test`, which commonly wrap helper functions and test cases in the same namespace.

That said, I agree `TestWalletName()` should probably be marked `static` to indicate internal linkag
...

📝 l0rinc opened a pull request: "refactor: inline constant return values from `dbwrapper` write methods"
(https://github.com/bitcoin/bitcoin/pull/33042)
Related to https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2223587480

### Summary
`WriteBatch` always returns `true` - the errors are handled by throwing `dbwrapper_error` instead.

### Context
This boolean return value of the `Write` methods is confusing because it's inconsistent with `CDBWrapper::Read`, which catches exceptions and returns a boolean to indicate success/failure. It's bad that `Read` returns and `Write` throws - but it's a lot worse that `Write` advertises a ret
...

💬 pablomartin4btc commented on pull request "rpc, test: Fix JSON parsing errors in unloadwallet and getdescriptoractivity RPCs":
(https://github.com/bitcoin/bitcoin/pull/32845#discussion_r2224178196)
Done!

💬 pablomartin4btc commented on pull request "rpc, test: Fix JSON parsing errors in unloadwallet and getdescriptoractivity RPCs":
(https://github.com/bitcoin/bitcoin/pull/32845#discussion_r2224178345)
Done!

💬 pablomartin4btc commented on pull request "rpc, test: Fix JSON parsing errors in unloadwallet and getdescriptoractivity RPCs":
(https://github.com/bitcoin/bitcoin/pull/32845#discussion_r2224178469)
Done!

💬 pablomartin4btc commented on pull request "rpc, test: Fix JSON parsing errors in unloadwallet and getdescriptoractivity RPCs":
(https://github.com/bitcoin/bitcoin/pull/32845#issuecomment-3105471281)
Addressed [feedback](https://github.com/bitcoin/bitcoin/pull/32845#pullrequestreview-3044915254) from @stickies-v.

💬 ajtowns commented on pull request "Cluster linearization: separate tests from tests-of-tests":
(https://github.com/bitcoin/bitcoin/pull/30605#discussion_r2224314216)
`if (!mask) mask += nonzero;` would be simpler, no? Or would that bias the fuzzer in undesirable ways somehow? Could perhaps do `mask += (!mask & nonzero)` to avoid an explicit branch.

🤔 yuvicc reviewed a pull request: "doc: remove dangling toc entries from `developer-notes.md`"
(https://github.com/bitcoin/bitcoin/pull/33040#pullrequestreview-3045563685)
I think we can also add

```
- [Interface Naming Style]([Internal interface
naming style](#internal-interface-naming-style))
```

👍 vasild approved a pull request: "net: Fix Discover() not running when using -bind=0.0.0.0:port"
(https://github.com/bitcoin/bitcoin/pull/32757#pullrequestreview-3045583278)
ACK 1dc6b1e8b04363107b4b44cbf42f8a25f2efabfe

Thank you!

💬 ajtowns commented on pull request "refactor: GenTxid type safety followups":
(https://github.com/bitcoin/bitcoin/pull/33005#discussion_r2224346894)
You might consider:

```diff

diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 10fbae5e7ed..42102973c31 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -947,7 +947,7 @@ private:
std::atomic<std::chrono::seconds> m_last_tip_update{0s};

/** Determine whether or not a peer can request a transaction, and return it (or nullptr if not found or not allowed). */
- CTransactionRef FindTxForGetData(const Peer::TxRelay& tx_relay, const CInv& i
...

💬 ajtowns commented on pull request "refactor: GenTxid type safety followups":
(https://github.com/bitcoin/bitcoin/pull/33005#discussion_r2224354639)
Or having `GetIter(GenTxid)` could work too:

```c++
```

💬 luisschwab commented on pull request "p2p: rename GetAddresses -> GetAddressesUnsafe":
(https://github.com/bitcoin/bitcoin/pull/32994#issuecomment-3105731244)
ACK 1cb23997033c395d9ecd7bf2f54787b134485f41

💬 maflcko commented on pull request "doc: update toc entries in `developer-notes.md`":
(https://github.com/bitcoin/bitcoin/pull/33040#issuecomment-3105890875)
For reference, GitHub already generates those: https://github.blog/changelog/2021-04-13-table-of-contents-support-in-markdown-files/

So I guess this is for local usage? Seems fine, but instead of manually updating and reviewing it, it would be better to do with a script. There are many tools (https://github.com/search?q=markdown%20toc&type=repositories) or standalone scripts, such as https://github.com/openai/codex/blob/d6c4083f98094dbd8d86f70992729924d93f73c4/scripts/readme_toc.py#L1.

💬 l0rinc commented on pull request "doc: update toc entries in `developer-notes.md`":
(https://github.com/bitcoin/bitcoin/pull/33040#issuecomment-3105905953)
We can also just delete the whole TOC - but keeping it half-updated seems counter-productive to me

💬 maflcko commented on pull request "doc: update toc entries in `developer-notes.md`":
(https://github.com/bitcoin/bitcoin/pull/33040#discussion_r2224549091)
Updating those also seems busy-work and inconsistent with the other headings (`#`). My preference would be to just use `#` consistently and not switch between `=`, `-`, and `#` in the same file.

💬 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"?