💬 stickies-v commented on pull request "p2p: rename GetAddresses -> GetAddressesUncached":
(https://github.com/bitcoin/bitcoin/pull/32994#discussion_r2219505959)
nit: documentation could be made more consistent, as per e.g. the below diff
<details>
<summary>git diff on df65b527f0</summary>
```diff
diff --git a/src/net.h b/src/net.h
index 61b25bd47d..eb08d9de05 100644
--- a/src/net.h
+++ b/src/net.h
@@ -1169,9 +1169,10 @@ public:
// Addrman functions
/**
- * Return all or many randomly selected addresses, optionally by network.
- * This function does not use an address response cache and should only be
- * used in
...
(https://github.com/bitcoin/bitcoin/pull/32994#discussion_r2219505959)
nit: documentation could be made more consistent, as per e.g. the below diff
<details>
<summary>git diff on df65b527f0</summary>
```diff
diff --git a/src/net.h b/src/net.h
index 61b25bd47d..eb08d9de05 100644
--- a/src/net.h
+++ b/src/net.h
@@ -1169,9 +1169,10 @@ public:
// Addrman functions
/**
- * Return all or many randomly selected addresses, optionally by network.
- * This function does not use an address response cache and should only be
- * used in
...
💬 darosior commented on pull request "[29.x] Backport #32521":
(https://github.com/bitcoin/bitcoin/pull/33013#issuecomment-3097189377)
Sure, will do.
(https://github.com/bitcoin/bitcoin/pull/33013#issuecomment-3097189377)
Sure, will do.
💬 stickies-v commented on pull request "p2p: rename GetAddresses -> GetAddressesUncached":
(https://github.com/bitcoin/bitcoin/pull/32994#discussion_r2219514574)
`GetAddressesForPeer` might also make sense. I don't really care about it too much, I think all the above options are all fine and the most important thing for me is that they're not the same, which is addressed, so would personally prefer to pick one thing and then move on.
(https://github.com/bitcoin/bitcoin/pull/32994#discussion_r2219514574)
`GetAddressesForPeer` might also make sense. I don't really care about it too much, I think all the above options are all fine and the most important thing for me is that they're not the same, which is addressed, so would personally prefer to pick one thing and then move on.
👋 glozow's pull request is ready for review: "p2p: TxOrphanage revamp cleanups"
(https://github.com/bitcoin/bitcoin/pull/32941)
(https://github.com/bitcoin/bitcoin/pull/32941)
💬 glozow commented on pull request "p2p: TxOrphanage revamp cleanups":
(https://github.com/bitcoin/bitcoin/pull/32941#issuecomment-3097204926)
This is ready for review. Would also be great to get https://github.com/bitcoin-core/qa-assets/pull/231 in for the 2 new fuzzies
(https://github.com/bitcoin/bitcoin/pull/32941#issuecomment-3097204926)
This is ready for review. Would also be great to get https://github.com/bitcoin-core/qa-assets/pull/231 in for the 2 new fuzzies
💬 marcofleon commented on pull request "p2p: TxOrphanage revamp cleanups":
(https://github.com/bitcoin/bitcoin/pull/32941#issuecomment-3097235779)
Looks like the `txorphanage_sim` inputs might be invalidated after this PR. Not sure if we should wait to add those inputs until after this gets in or if it's worth adding them before.
(https://github.com/bitcoin/bitcoin/pull/32941#issuecomment-3097235779)
Looks like the `txorphanage_sim` inputs might be invalidated after this PR. Not sure if we should wait to add those inputs until after this gets in or if it's worth adding them before.
💬 mzumsande commented on pull request "p2p: Advance pindexLastCommonBlock early after connecting blocks":
(https://github.com/bitcoin/bitcoin/pull/32180#issuecomment-3097243452)
[b7ff6a6](https://github.com/bitcoin/bitcoin/commit/b7ff6a611a220e9380f6cd6428f1d3483c8d566f) to [3bdff9a](https://github.com/bitcoin/bitcoin/commit/3bdff9a154733f8f9f379448f5595a2e90474bc6): rebased due to conflict with #32868
(https://github.com/bitcoin/bitcoin/pull/32180#issuecomment-3097243452)
[b7ff6a6](https://github.com/bitcoin/bitcoin/commit/b7ff6a611a220e9380f6cd6428f1d3483c8d566f) to [3bdff9a](https://github.com/bitcoin/bitcoin/commit/3bdff9a154733f8f9f379448f5595a2e90474bc6): rebased due to conflict with #32868
📝 naiyoma opened a pull request: "net: make m_mempool optional in PeerManager "
(https://github.com/bitcoin/bitcoin/pull/33029)
This PR is a continuation of →https://github.com/bitcoin/bitcoin/pull/22850, to make m_mempool an optional pointer in PeerManager instead of a reference.
some benefits, as mentioned here:https://github.com/bitcoin/bitcoin/pull/26247#issue-1396302764 are:
1. m_mempool is already a pointer in other parts like:
In Chainstate:[ https://github.com/bitcoin/bitcoin/blob/master/src/validation.h#L532](https://github.com/bitcoin/bitcoin/blob/master/src/validation.h#L532)
In BlockAssembler:[ https
...
(https://github.com/bitcoin/bitcoin/pull/33029)
This PR is a continuation of →https://github.com/bitcoin/bitcoin/pull/22850, to make m_mempool an optional pointer in PeerManager instead of a reference.
some benefits, as mentioned here:https://github.com/bitcoin/bitcoin/pull/26247#issue-1396302764 are:
1. m_mempool is already a pointer in other parts like:
In Chainstate:[ https://github.com/bitcoin/bitcoin/blob/master/src/validation.h#L532](https://github.com/bitcoin/bitcoin/blob/master/src/validation.h#L532)
In BlockAssembler:[ https
...
💬 glozow commented on pull request "p2p: TxOrphanage revamp cleanups":
(https://github.com/bitcoin/bitcoin/pull/32941#issuecomment-3097406288)
> Looks like the txorphanage_sim inputs might be invalidated with this PR.
Ah good point, yeah.
(https://github.com/bitcoin/bitcoin/pull/32941#issuecomment-3097406288)
> Looks like the txorphanage_sim inputs might be invalidated with this PR.
Ah good point, yeah.
💬 sipa commented on pull request "p2p: TxOrphanage revamp cleanups":
(https://github.com/bitcoin/bitcoin/pull/32941#discussion_r2219698256)
Use a scripted diff?
(https://github.com/bitcoin/bitcoin/pull/32941#discussion_r2219698256)
Use a scripted diff?
💬 sipa commented on pull request "p2p: TxOrphanage revamp cleanups":
(https://github.com/bitcoin/bitcoin/pull/32941#discussion_r2219704726)
That's a lot of fuzzer bytes (32) for very little gain I think. Since it's already fed through a cryptographic RNG anyway, I don't think consuming bytes from the fuzzer has any advantage over using the harness' `rng` (which is already seeded by fuzz input anyway).
(https://github.com/bitcoin/bitcoin/pull/32941#discussion_r2219704726)
That's a lot of fuzzer bytes (32) for very little gain I think. Since it's already fed through a cryptographic RNG anyway, I don't think consuming bytes from the fuzzer has any advantage over using the harness' `rng` (which is already seeded by fuzz input anyway).
💬 sipa commented on pull request "p2p: TxOrphanage revamp cleanups":
(https://github.com/bitcoin/bitcoin/pull/32941#discussion_r2219589388)
Hmm, this means that anyone with a `bitcoin.conf` that lists `maxorphantxs=N` will now get an error. An alternative could be to leave the option in for a while, but make it just emit a warning. Unsure if that's necessary; I doubt many people configure this, but I don't remember how we've dealt with similar changes in recent history.
(https://github.com/bitcoin/bitcoin/pull/32941#discussion_r2219589388)
Hmm, this means that anyone with a `bitcoin.conf` that lists `maxorphantxs=N` will now get an error. An alternative could be to leave the option in for a while, but make it just emit a warning. Unsure if that's necessary; I doubt many people configure this, but I don't remember how we've dealt with similar changes in recent history.
💬 sipa commented on pull request "p2p: TxOrphanage revamp cleanups":
(https://github.com/bitcoin/bitcoin/pull/32941#discussion_r2219699199)
Use a scripted-diff?
(https://github.com/bitcoin/bitcoin/pull/32941#discussion_r2219699199)
Use a scripted-diff?
💬 l0rinc commented on pull request "p2p: rename GetAddresses -> GetAddressesUncached":
(https://github.com/bitcoin/bitcoin/pull/32994#discussion_r2219731173)
Actually, first seeing `Uncached` in a name implies to me that it's safer, since it doesn't have side-effects, doesn't pollute the context, it cannot blow up, etc - I don't think it achieves scaring people away.
In other cases the suffix was "Unsafe" if it's important to signal that the user should pay extra attention.
(https://github.com/bitcoin/bitcoin/pull/32994#discussion_r2219731173)
Actually, first seeing `Uncached` in a name implies to me that it's safer, since it doesn't have side-effects, doesn't pollute the context, it cannot blow up, etc - I don't think it achieves scaring people away.
In other cases the suffix was "Unsafe" if it's important to signal that the user should pay extra attention.
💬 Zeegaths commented on pull request "docs: adds correct updated documentation links":
(https://github.com/bitcoin/bitcoin/pull/32699#issuecomment-3097660338)
thanks for pointing that out. all good now
On Mon, 21 Jul 2025 at 17:19, maflcko ***@***.***> wrote:
> *maflcko* left a comment (bitcoin/bitcoin#32699)
> <https://github.com/bitcoin/bitcoin/pull/32699#issuecomment-3096984862>
>
> No, you haven't replied/addressed the feedback from June: #32699 (comment)
> <https://github.com/bitcoin/bitcoin/pull/32699#discussion_r2150261914>
>
> —
> Reply to this email directly, view it on GitHub
> <https://github.com/bitcoin/bitcoin/pull/32699#iss
...
(https://github.com/bitcoin/bitcoin/pull/32699#issuecomment-3097660338)
thanks for pointing that out. all good now
On Mon, 21 Jul 2025 at 17:19, maflcko ***@***.***> wrote:
> *maflcko* left a comment (bitcoin/bitcoin#32699)
> <https://github.com/bitcoin/bitcoin/pull/32699#issuecomment-3096984862>
>
> No, you haven't replied/addressed the feedback from June: #32699 (comment)
> <https://github.com/bitcoin/bitcoin/pull/32699#discussion_r2150261914>
>
> —
> Reply to this email directly, view it on GitHub
> <https://github.com/bitcoin/bitcoin/pull/32699#iss
...
👋 Crypt-iQ's pull request is ready for review: "log: rate limiting followups"
(https://github.com/bitcoin/bitcoin/pull/33011)
(https://github.com/bitcoin/bitcoin/pull/33011)
💬 tnndbtc commented on issue "intermittent timeout in wallet_signer.py : 'createwallet' RPC took longer than 1200.000000 seconds":
(https://github.com/bitcoin/bitcoin/issues/32855#issuecomment-3097892555)
@naiyoma For `if(str_std_in.empty())`, it will always return true in this case. However, the empty string is sent by successful calling of `subprocess_close(err_wr_pipe);// close child side of pipe, else get stuck in read below`. If you comment out this line, you will see that read from stdin in signer.py will 100% get stuck.
(https://github.com/bitcoin/bitcoin/issues/32855#issuecomment-3097892555)
@naiyoma For `if(str_std_in.empty())`, it will always return true in this case. However, the empty string is sent by successful calling of `subprocess_close(err_wr_pipe);// close child side of pipe, else get stuck in read below`. If you comment out this line, you will see that read from stdin in signer.py will 100% get stuck.
🤔 l0rinc reviewed a pull request: "[IBD] prevector: store `P2WSH`/`P2TR`/`P2PK` scripts inline"
(https://github.com/bitcoin/bitcoin/pull/32279#pullrequestreview-3039141524)
Thanks, applied most of your comments + a few additional cleanups.
The first push was a simple rebase, the second contains the changes.
(https://github.com/bitcoin/bitcoin/pull/32279#pullrequestreview-3039141524)
Thanks, applied most of your comments + a few additional cleanups.
The first push was a simple rebase, the second contains the changes.
💬 l0rinc commented on pull request "[IBD] prevector: store `P2WSH`/`P2TR`/`P2PK` scripts inline":
(https://github.com/bitcoin/bitcoin/pull/32279#discussion_r2219879016)
Sure, modernized the definition in a separate commit
(https://github.com/bitcoin/bitcoin/pull/32279#discussion_r2219879016)
Sure, modernized the definition in a separate commit
💬 l0rinc commented on pull request "[IBD] prevector: store `P2WSH`/`P2TR`/`P2PK` scripts inline":
(https://github.com/bitcoin/bitcoin/pull/32279#discussion_r2219893434)
Done
(https://github.com/bitcoin/bitcoin/pull/32279#discussion_r2219893434)
Done