💬 maflcko commented on pull request "Lint: support running individual rust linters and improve subtree exclusion":
(https://github.com/bitcoin/bitcoin/pull/29965#discussion_r1583306274)
nit in 88b824a80b5c955285ef2ddc490e086bb40a890c: Given that you are adding features to the lint runner which are not trivially available in the docker way (above), it could make sense to move the docker option down by a section (only as a fallback), as it seems to have bugs anyway, according to https://github.com/bitcoin/bitcoin/pull/29494#issuecomment-2083018272
(https://github.com/bitcoin/bitcoin/pull/29965#discussion_r1583306274)
nit in 88b824a80b5c955285ef2ddc490e086bb40a890c: Given that you are adding features to the lint runner which are not trivially available in the docker way (above), it could make sense to move the docker option down by a section (only as a fallback), as it seems to have bugs anyway, according to https://github.com/bitcoin/bitcoin/pull/29494#issuecomment-2083018272
💬 stickies-v commented on pull request "doc: removed help text saying that peers may not connect automatically":
(https://github.com/bitcoin/bitcoin/pull/29994#discussion_r1583309265)
I don't think we should remove default ports or the I2P doc
```suggestion
argsman.AddArg("-port=<port>", strprintf("Listen for connections on <port> (default: %u, testnet: %u, signet: %u, regtest: %u). Not relevant for I2P (see doc/i2p.md).", defaultChainParams->GetDefaultPort(), testnetChainParams->GetDefaultPort(), signetChainParams->GetDefaultPort(), regtestChainParams->GetDefaultPort()), ArgsManager::ALLOW_ANY | ArgsManager::NETWORK_ONLY, OptionsCategory::CONNECTION);
```
(https://github.com/bitcoin/bitcoin/pull/29994#discussion_r1583309265)
I don't think we should remove default ports or the I2P doc
```suggestion
argsman.AddArg("-port=<port>", strprintf("Listen for connections on <port> (default: %u, testnet: %u, signet: %u, regtest: %u). Not relevant for I2P (see doc/i2p.md).", defaultChainParams->GetDefaultPort(), testnetChainParams->GetDefaultPort(), signetChainParams->GetDefaultPort(), regtestChainParams->GetDefaultPort()), ArgsManager::ALLOW_ANY | ArgsManager::NETWORK_ONLY, OptionsCategory::CONNECTION);
```
🤔 stickies-v reviewed a pull request: "doc: removed help text saying that peers may not connect automatically"
(https://github.com/bitcoin/bitcoin/pull/29994#pullrequestreview-2028885329)
Concept ACK. According to https://bitnodes.io/dashboard/, ~85% of public nodes are running 23 or higher.
(https://github.com/bitcoin/bitcoin/pull/29994#pullrequestreview-2028885329)
Concept ACK. According to https://bitnodes.io/dashboard/, ~85% of public nodes are running 23 or higher.
📝 instagibbs opened a pull request: "functional test: ensure confirmed utxo being sourced for 2nd chain"
(https://github.com/bitcoin/bitcoin/pull/29998)
The test could fail/stop testing what we want if non-confirmed utxos become sourced through some internal change to `MiniWallet`; better to just fetch confirmed explicitly.
(https://github.com/bitcoin/bitcoin/pull/29998)
The test could fail/stop testing what we want if non-confirmed utxos become sourced through some internal change to `MiniWallet`; better to just fetch confirmed explicitly.
💬 itornaza commented on pull request "p2p: gives seednode priority over dnsseed if both are provided":
(https://github.com/bitcoin/bitcoin/pull/28016#issuecomment-2083124545)
tested re-ACK 82f41d76f1c6ad38290917dad5499ffbe6b3974d
This PR not only adds meaningful functionality by enabling seeds that are provided by the user to take precedence over the default DNS seeds, but also enhances user experience with relevant reporting on what exactly happens through the process of connecting to the P2P network and which type of nodes are effectively used.
Built this PR from a cleaned up environment using `make distclean` and checked out the latest commit to test upon. M
...
(https://github.com/bitcoin/bitcoin/pull/28016#issuecomment-2083124545)
tested re-ACK 82f41d76f1c6ad38290917dad5499ffbe6b3974d
This PR not only adds meaningful functionality by enabling seeds that are provided by the user to take precedence over the default DNS seeds, but also enhances user experience with relevant reporting on what exactly happens through the process of connecting to the P2P network and which type of nodes are effectively used.
Built this PR from a cleaned up environment using `make distclean` and checked out the latest commit to test upon. M
...
💬 sdaftuar commented on pull request "test: Don't rely on incentive incompatible replacement in mempool_accept_v3.py":
(https://github.com/bitcoin/bitcoin/pull/29986#discussion_r1583360322)
I think that works, let me know if what I pushed matches what you're thinking!
(https://github.com/bitcoin/bitcoin/pull/29986#discussion_r1583360322)
I think that works, let me know if what I pushed matches what you're thinking!
👍 instagibbs approved a pull request: "test: Don't rely on incentive incompatible replacement in mempool_accept_v3.py"
(https://github.com/bitcoin/bitcoin/pull/29986#pullrequestreview-2028979977)
ACK f8a141c2dae2471a7ce7248e28a0bbeb8a291acd
(https://github.com/bitcoin/bitcoin/pull/29986#pullrequestreview-2028979977)
ACK f8a141c2dae2471a7ce7248e28a0bbeb8a291acd
👍 instagibbs approved a pull request: "fuzz: txorphan tests fixups"
(https://github.com/bitcoin/bitcoin/pull/29974#pullrequestreview-2028986127)
ACK 6998f02c97c0b2c3579fac160192402c8b09613d
(https://github.com/bitcoin/bitcoin/pull/29974#pullrequestreview-2028986127)
ACK 6998f02c97c0b2c3579fac160192402c8b09613d
💬 stickies-v commented on pull request "rpc: return warnings as an array instead of just a single one":
(https://github.com/bitcoin/bitcoin/pull/29845#discussion_r1583373054)
> Happy to have a go after this gets merged.
Implemented in https://github.com/bitcoin/bitcoin/compare/master...stickies-v:bitcoin:2024-04/move-warnings-node
(https://github.com/bitcoin/bitcoin/pull/29845#discussion_r1583373054)
> Happy to have a go after this gets merged.
Implemented in https://github.com/bitcoin/bitcoin/compare/master...stickies-v:bitcoin:2024-04/move-warnings-node
💬 fjahr commented on pull request "prune, rpc: Check undo data when finding pruneheight":
(https://github.com/bitcoin/bitcoin/pull/29668#discussion_r1583375437)
done
(https://github.com/bitcoin/bitcoin/pull/29668#discussion_r1583375437)
done
💬 fjahr commented on pull request "prune, rpc: Check undo data when finding pruneheight":
(https://github.com/bitcoin/bitcoin/pull/29668#discussion_r1583375522)
done
(https://github.com/bitcoin/bitcoin/pull/29668#discussion_r1583375522)
done
💬 fjahr commented on pull request "prune, rpc: Check undo data when finding pruneheight":
(https://github.com/bitcoin/bitcoin/pull/29668#discussion_r1583375709)
done
(https://github.com/bitcoin/bitcoin/pull/29668#discussion_r1583375709)
done
💬 fjahr commented on pull request "prune, rpc: Check undo data when finding pruneheight":
(https://github.com/bitcoin/bitcoin/pull/29668#discussion_r1583375852)
added
(https://github.com/bitcoin/bitcoin/pull/29668#discussion_r1583375852)
added
💬 fjahr commented on pull request "prune, rpc: Check undo data when finding pruneheight":
(https://github.com/bitcoin/bitcoin/pull/29668#discussion_r1583376000)
taken
(https://github.com/bitcoin/bitcoin/pull/29668#discussion_r1583376000)
taken
💬 fjahr commented on pull request "prune, rpc: Check undo data when finding pruneheight":
(https://github.com/bitcoin/bitcoin/pull/29668#discussion_r1583376085)
I prefer to keep this one as is because I find it more intuitive.
(https://github.com/bitcoin/bitcoin/pull/29668#discussion_r1583376085)
I prefer to keep this one as is because I find it more intuitive.
💬 fjahr commented on pull request "prune, rpc: Check undo data when finding pruneheight":
(https://github.com/bitcoin/bitcoin/pull/29668#issuecomment-2083168939)
Adressed feedback from @stickies-v , thanks again!
> nit: I think the last 3 commits should be squashed
The doc change is only tangentially related so I will keep it separate. But I squashed the 2nd and 3rd.
(https://github.com/bitcoin/bitcoin/pull/29668#issuecomment-2083168939)
Adressed feedback from @stickies-v , thanks again!
> nit: I think the last 3 commits should be squashed
The doc change is only tangentially related so I will keep it separate. But I squashed the 2nd and 3rd.
💬 glozow commented on pull request "test: Don't rely on incentive incompatible replacement in mempool_accept_v3.py":
(https://github.com/bitcoin/bitcoin/pull/29986#issuecomment-2083176753)
ACK f8a141c2dae2471a7ce7248e28a0bbeb8a291acd
(https://github.com/bitcoin/bitcoin/pull/29986#issuecomment-2083176753)
ACK f8a141c2dae2471a7ce7248e28a0bbeb8a291acd
💬 glozow commented on pull request "fuzz: don't allow adding duplicate transactions to the mempool":
(https://github.com/bitcoin/bitcoin/pull/29990#issuecomment-2083179739)
utACK cc15c5bfd1eb3903de246c124702a7c66c687008 makes sense to me
(https://github.com/bitcoin/bitcoin/pull/29990#issuecomment-2083179739)
utACK cc15c5bfd1eb3903de246c124702a7c66c687008 makes sense to me
💬 kevkevinpal commented on pull request "doc: removed help text saying that peers may not connect automatically":
(https://github.com/bitcoin/bitcoin/pull/29994#discussion_r1583385359)
fixed in [95897ff](https://github.com/bitcoin/bitcoin/pull/29994/commits/95897ff181c0757e445f0e066a2a590a0a0120d2)
(https://github.com/bitcoin/bitcoin/pull/29994#discussion_r1583385359)
fixed in [95897ff](https://github.com/bitcoin/bitcoin/pull/29994/commits/95897ff181c0757e445f0e066a2a590a0a0120d2)
💬 denavila commented on pull request "Deniability - a tool to automatically improve coin ownership privacy":
(https://github.com/bitcoin-core/gui/pull/733#issuecomment-2083190086)
@1440000bytes,
Thank you so much for testing the PR and for the feedback. That's very helpful.
(1) Yes, that's good idea, I'll rename it.
(2) and (5) are related. The selectable frequencies are actually also randomized, within the selected frequency range - for example hourly is random from 1 to 2 hours, daily is random from 1 to 2 days etc (see line 934 in deniabilitydialog.cpp). Perhaps we need to rename the frequency UI selection to something that better describes this?
(3) Great idea
...
(https://github.com/bitcoin-core/gui/pull/733#issuecomment-2083190086)
@1440000bytes,
Thank you so much for testing the PR and for the feedback. That's very helpful.
(1) Yes, that's good idea, I'll rename it.
(2) and (5) are related. The selectable frequencies are actually also randomized, within the selected frequency range - for example hourly is random from 1 to 2 hours, daily is random from 1 to 2 days etc (see line 934 in deniabilitydialog.cpp). Perhaps we need to rename the frequency UI selection to something that better describes this?
(3) Great idea
...
💬 hebasto commented on pull request "kernel: Streamline util library":
(https://github.com/bitcoin/bitcoin/pull/29015#issuecomment-2083201174)
I've tested 5eeafeb302dcf56dd5c16c1e4785a41a132344c7 on Ubuntu 23.10.
While all changes look correct and reasonable to me, I was expecting that this PR removes some of dependencies mentioned in https://github.com/bitcoin/bitcoin/issues/28548.
But that is not the case. The `libbitcoin_util` still depends on `libbitcoin_crypto`, `libbitcoin_consensus` and `libbitcoin_common` libraries.
If such an outcome is expected, maybe update the `doc/design/libraries.md` accordingly?
(https://github.com/bitcoin/bitcoin/pull/29015#issuecomment-2083201174)
I've tested 5eeafeb302dcf56dd5c16c1e4785a41a132344c7 on Ubuntu 23.10.
While all changes look correct and reasonable to me, I was expecting that this PR removes some of dependencies mentioned in https://github.com/bitcoin/bitcoin/issues/28548.
But that is not the case. The `libbitcoin_util` still depends on `libbitcoin_crypto`, `libbitcoin_consensus` and `libbitcoin_common` libraries.
If such an outcome is expected, maybe update the `doc/design/libraries.md` accordingly?