💬 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.
(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))
```
(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!
(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
...
(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++
```
(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
(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.
(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
(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.
(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.
(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
(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
(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
(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
(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"?
(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?
(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
(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
(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.
(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
...
(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
...