💬 vasild commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1214128768)
I removed `new_p2p_index()` because it was used in just one place. Earlier it was used also for all listeners behind the SOCKS5 proxy, but I had to stop using `p2p_port()` for that because it exceeded `MAX_NODES` (12).
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1214128768)
I removed `new_p2p_index()` because it was used in just one place. Earlier it was used also for all listeners behind the SOCKS5 proxy, but I had to stop using `p2p_port()` for that because it exceeded `MAX_NODES` (12).
💬 vasild commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1214131440)
Yes. That is a bit hidden/implicit in the creation of the grant, in `master`:
```cpp
CSemaphoreGrant grant(*semOutbound);
```
this would hang if there are too many connections, waiting for a free slot to be available (somebody to disconnect).
Added a mention to it in the description of `-maxconnections`.
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1214131440)
Yes. That is a bit hidden/implicit in the creation of the grant, in `master`:
```cpp
CSemaphoreGrant grant(*semOutbound);
```
this would hang if there are too many connections, waiting for a free slot to be available (somebody to disconnect).
Added a mention to it in the description of `-maxconnections`.
🚀 fanquake merged a pull request: "streams: Drop confusing DataStream::Serialize method and << operator"
(https://github.com/bitcoin/bitcoin/pull/27800)
(https://github.com/bitcoin/bitcoin/pull/27800)
💬 fanquake commented on pull request "ci: compile Clang and compiler-rt in msan jobs":
(https://github.com/bitcoin/bitcoin/pull/27737#issuecomment-1573450367)
Using alternate distros, or the pre-compiled LLVM bins do not currently seem to be viable options. Follow up in the qa-assets repo is here: https://github.com/bitcoin-core/qa-assets/pull/129.
(https://github.com/bitcoin/bitcoin/pull/27737#issuecomment-1573450367)
Using alternate distros, or the pre-compiled LLVM bins do not currently seem to be viable options. Follow up in the qa-assets repo is here: https://github.com/bitcoin-core/qa-assets/pull/129.
🚀 fanquake merged a pull request: "ci: compile Clang and compiler-rt in msan jobs"
(https://github.com/bitcoin/bitcoin/pull/27737)
(https://github.com/bitcoin/bitcoin/pull/27737)
🤔 glozow reviewed a pull request: "Fuzz: Mitigate timeout in CalculateTotalBumpFees"
(https://github.com/bitcoin/bitcoin/pull/27803#pullrequestreview-1457035240)
ACK 5d718f6913219d3ebe8394a17ddee81915e6f0ac
The code changes make sense to me. Reproduced the slow input from #27799, and it is dramatically faster with this change.
(https://github.com/bitcoin/bitcoin/pull/27803#pullrequestreview-1457035240)
ACK 5d718f6913219d3ebe8394a17ddee81915e6f0ac
The code changes make sense to me. Reproduced the slow input from #27799, and it is dramatically faster with this change.
✅ glozow closed an issue: "fuzz: mini_miner: Timeout in mini_miner"
(https://github.com/bitcoin/bitcoin/issues/27799)
(https://github.com/bitcoin/bitcoin/issues/27799)
🚀 glozow merged a pull request: "Fuzz: Mitigate timeout in CalculateTotalBumpFees"
(https://github.com/bitcoin/bitcoin/pull/27803)
(https://github.com/bitcoin/bitcoin/pull/27803)
💬 willcl-ark commented on pull request "Fee estimation: avoid serving stale fee estimate":
(https://github.com/bitcoin/bitcoin/pull/27622#discussion_r1214180938)
Can you explain the rationale behind the 60 hours chosen? Also perhaps you can update OP as it still mentions 12 hours :)
(https://github.com/bitcoin/bitcoin/pull/27622#discussion_r1214180938)
Can you explain the rationale behind the 60 hours chosen? Also perhaps you can update OP as it still mentions 12 hours :)
💬 theStack commented on pull request "rpc: remove deprecated "warning" field from {create,load,restore,unload}wallet":
(https://github.com/bitcoin/bitcoin/pull/27757#discussion_r1214199866)
Thanks, included the move back of the `UnloadWallet` call and added you as co-author.
(https://github.com/bitcoin/bitcoin/pull/27757#discussion_r1214199866)
Thanks, included the move back of the `UnloadWallet` call and added you as co-author.
💬 theStack commented on pull request "rpc: remove deprecated "warning" field from {create,load,restore,unload}wallet":
(https://github.com/bitcoin/bitcoin/pull/27757#discussion_r1214200031)
Thanks, done.
(https://github.com/bitcoin/bitcoin/pull/27757#discussion_r1214200031)
Thanks, done.
💬 theStack commented on pull request "rpc: remove deprecated "warning" field from {create,load,restore,unload}wallet":
(https://github.com/bitcoin/bitcoin/pull/27757#issuecomment-1573510436)
@jonatack: Oh sorry, wasn't aware you already put time into this. Pulled in your documentation fixup commit and took also your other suggestions for the removal and release note commits.
(https://github.com/bitcoin/bitcoin/pull/27757#issuecomment-1573510436)
@jonatack: Oh sorry, wasn't aware you already put time into this. Pulled in your documentation fixup commit and took also your other suggestions for the removal and release note commits.
💬 glozow commented on pull request "[NO MERGE] BIP331 Ancestor Package Relay":
(https://github.com/bitcoin/bitcoin/pull/27742#issuecomment-1573521731)
> > redownloading orphans if we cannot afford to protect them
>
> Is the idea here that protection is a bandiwdth optimization to avoid fetching an orphan twice in a row in the presence of malicious peers or random churn? Reasonable if so, just want this to be explicitly stated if so.
Yes exactly. Worse is we want to avoid a situation where we requested the rest of the package while the orphan was in our orphanage, but then it fell out while we were waiting for the rest of the transactions
...
(https://github.com/bitcoin/bitcoin/pull/27742#issuecomment-1573521731)
> > redownloading orphans if we cannot afford to protect them
>
> Is the idea here that protection is a bandiwdth optimization to avoid fetching an orphan twice in a row in the presence of malicious peers or random churn? Reasonable if so, just want this to be explicitly stated if so.
Yes exactly. Worse is we want to avoid a situation where we requested the rest of the package while the orphan was in our orphanage, but then it fell out while we were waiting for the rest of the transactions
...
💬 TheCharlatan commented on pull request "kernel: Remove shutdown from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27711#discussion_r1214254203)
I don't really understand the improvement in the context of achieving step 2 of the kernel library project between the solution you implemented now and what is implemented currently here in the first commit, besides it being a big step towards de-globalizing. One of my goals for this PR here is to stop the kernel relying on code that handles platform-dependent blocking while still being signal-aware (e.g. the bulk of what is currently in shutdown). I don't see why such logic should exist in a st
...
(https://github.com/bitcoin/bitcoin/pull/27711#discussion_r1214254203)
I don't really understand the improvement in the context of achieving step 2 of the kernel library project between the solution you implemented now and what is implemented currently here in the first commit, besides it being a big step towards de-globalizing. One of my goals for this PR here is to stop the kernel relying on code that handles platform-dependent blocking while still being signal-aware (e.g. the bulk of what is currently in shutdown). I don't see why such logic should exist in a st
...
💬 ChrisCho-H commented on pull request "script: add description for the functionality of each opcode":
(https://github.com/bitcoin/bitcoin/pull/27109#issuecomment-1573587610)
@MarcoFalke would u mind to review this?
(https://github.com/bitcoin/bitcoin/pull/27109#issuecomment-1573587610)
@MarcoFalke would u mind to review this?
💬 ryanofsky commented on pull request "walletdb: Add PrefixCursor":
(https://github.com/bitcoin/bitcoin/pull/27790#discussion_r1214272570)
I think I agree that current `Serialize(Span<char>)` overloads could be a little dangerous:
https://github.com/bitcoin/bitcoin/blob/6a560aceb75e618f3106a8850e053cd8de87616a/src/serialize.h#L202-L205
Because if a variable-length container could be implicitly converted to span of these types, it might incorrectly match one of these overloads and be serialized as a fixed length span without a length prefix, and no longer be deserializable as the original container.
If that is a real proble
...
(https://github.com/bitcoin/bitcoin/pull/27790#discussion_r1214272570)
I think I agree that current `Serialize(Span<char>)` overloads could be a little dangerous:
https://github.com/bitcoin/bitcoin/blob/6a560aceb75e618f3106a8850e053cd8de87616a/src/serialize.h#L202-L205
Because if a variable-length container could be implicitly converted to span of these types, it might incorrectly match one of these overloads and be serialized as a fixed length span without a length prefix, and no longer be deserializable as the original container.
If that is a real proble
...
🤔 Sjors reviewed a pull request: "contrib: verify torrents with verify-binary.py"
(https://github.com/bitcoin/bitcoin/pull/27762#pullrequestreview-1457289222)
Successfully tested on macOS 13.4 against v25.0.
Lightly code reviewed a6dcf2cb0817143beacf0b9004191a0dd6d2e43c.
Do we want to use the .asc file from the torrent, or should we just get it from the Guix signature repo? The latter may have more signatures, as they keep coming in after release.
(https://github.com/bitcoin/bitcoin/pull/27762#pullrequestreview-1457289222)
Successfully tested on macOS 13.4 against v25.0.
Lightly code reviewed a6dcf2cb0817143beacf0b9004191a0dd6d2e43c.
Do we want to use the .asc file from the torrent, or should we just get it from the Guix signature repo? The latter may have more signatures, as they keep coming in after release.
💬 Sjors commented on pull request "contrib: verify torrents with verify-binary.py":
(https://github.com/bitcoin/bitcoin/pull/27762#discussion_r1214312611)
a6dcf2cb0817143beacf0b9004191a0dd6d2e43c Did you mean to change the indentation here? Maybe do so in the first refactor commit?
(https://github.com/bitcoin/bitcoin/pull/27762#discussion_r1214312611)
a6dcf2cb0817143beacf0b9004191a0dd6d2e43c Did you mean to change the indentation here? Maybe do so in the first refactor commit?
💬 Sjors commented on pull request "contrib: verify torrents with verify-binary.py":
(https://github.com/bitcoin/bitcoin/pull/27762#discussion_r1214314041)
a6dcf2cb0817143beacf0b9004191a0dd6d2e43c: did you mean to do this in the refactor commit?
(https://github.com/bitcoin/bitcoin/pull/27762#discussion_r1214314041)
a6dcf2cb0817143beacf0b9004191a0dd6d2e43c: did you mean to do this in the refactor commit?
💬 ryanofsky commented on pull request "kernel: Remove shutdown from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27711#discussion_r1214342650)
> One of my goals for this PR here is to stop the kernel relying on code that handles platform-dependent blocking while still being signal-aware (e.g. the bulk of what is currently in shutdown). I don't see why such logic should exist in a standalone kernel library. Do you have a different perspective here?
Yes, I don't think we should do this because it gets in the way of the kernel having a nice and future-proof API.
I think we both agree that applications using libbitcoinkernel should h
...
(https://github.com/bitcoin/bitcoin/pull/27711#discussion_r1214342650)
> One of my goals for this PR here is to stop the kernel relying on code that handles platform-dependent blocking while still being signal-aware (e.g. the bulk of what is currently in shutdown). I don't see why such logic should exist in a standalone kernel library. Do you have a different perspective here?
Yes, I don't think we should do this because it gets in the way of the kernel having a nice and future-proof API.
I think we both agree that applications using libbitcoinkernel should h
...