💬 benthecarman commented on pull request "Allow configuring target block time for a signet":
(https://github.com/bitcoin/bitcoin/pull/27446#issuecomment-1505698929)
Pushed up some changes to respond to @stickies-v and @MarcoFalke's review
(https://github.com/bitcoin/bitcoin/pull/27446#issuecomment-1505698929)
Pushed up some changes to respond to @stickies-v and @MarcoFalke's review
💬 benthecarman commented on pull request "Allow configuring target block time for a signet":
(https://github.com/bitcoin/bitcoin/pull/27446#issuecomment-1505704805)
@ajtowns I don't think I am properly communicating my usecase correctly, but there are lots of concept acks from other developers that run signets as well. People want to use custom signets to have their own test networks, I don't see why disallowing having custom block times shouldn't be allowed. The main concern being people don't copy-paste the param into their conf and have troubles syncing, doesn't seem like a valid one given that they could do the same for `-signetchallenge`. I understand
...
(https://github.com/bitcoin/bitcoin/pull/27446#issuecomment-1505704805)
@ajtowns I don't think I am properly communicating my usecase correctly, but there are lots of concept acks from other developers that run signets as well. People want to use custom signets to have their own test networks, I don't see why disallowing having custom block times shouldn't be allowed. The main concern being people don't copy-paste the param into their conf and have troubles syncing, doesn't seem like a valid one given that they could do the same for `-signetchallenge`. I understand
...
💬 glozow commented on pull request "mempool: disallow txns under min relay fee, even in packages":
(https://github.com/bitcoin/bitcoin/pull/26933#issuecomment-1505710683)
> Would it make sense to replace this with a PR that enabled package relay without v3 or package RBF, and included this policy? I think that would be the first ~10 commits from https://github.com/bitcoin/bitcoin/pull/25038? That seems like it would be more meaningful progress...
Right, the changes for package relay only (i.e. no v3, no package RBF) would be this, persist packages across restart, and allow any ancestor package instead of just child-with-parents. Basically this PR + the first 7
...
(https://github.com/bitcoin/bitcoin/pull/26933#issuecomment-1505710683)
> Would it make sense to replace this with a PR that enabled package relay without v3 or package RBF, and included this policy? I think that would be the first ~10 commits from https://github.com/bitcoin/bitcoin/pull/25038? That seems like it would be more meaningful progress...
Right, the changes for package relay only (i.e. no v3, no package RBF) would be this, persist packages across restart, and allow any ancestor package instead of just child-with-parents. Basically this PR + the first 7
...
💬 mzumsande commented on pull request "p2p: Restrict self-advertisements with privacy networks to avoid fingerprinting":
(https://github.com/bitcoin/bitcoin/pull/27411#issuecomment-1505713147)
- Updated the PR to separate privacy checks from reachability checks.
- Passed a CNode to `GetLocal()` and replaced pointers with references in `GetLocal()` and `GetReachabilityFrom()`
- Added a unit test
(https://github.com/bitcoin/bitcoin/pull/27411#issuecomment-1505713147)
- Updated the PR to separate privacy checks from reachability checks.
- Passed a CNode to `GetLocal()` and replaced pointers with references in `GetLocal()` and `GetReachabilityFrom()`
- Added a unit test
📝 1440000bytes opened a pull request: "doc: remove incorrect line from example"
(https://github.com/bitcoin/bitcoin/pull/27454)
Inputs are always present when using `walletcreatefundedpsbt`
(https://github.com/bitcoin/bitcoin/pull/27454)
Inputs are always present when using `walletcreatefundedpsbt`
💬 earuak commented on pull request "test: added coverage to rpc_scantxoutset.py":
(https://github.com/bitcoin/bitcoin/pull/27453#issuecomment-1505715474)
The test is now run before starting an RPC procedure to ensure a valid RPC command is entered.
This change appears to be good programming practice, as it helps ensure that the system works correctly and that users receive immediate feedback if they enter an invalid command. RPC failing if "start", "status" or "abort" doesn't stop the first command also seems to be a proper way to handle the problem.
Overall, including rigorous unit testing is a best practice in software programming, especi
...
(https://github.com/bitcoin/bitcoin/pull/27453#issuecomment-1505715474)
The test is now run before starting an RPC procedure to ensure a valid RPC command is entered.
This change appears to be good programming practice, as it helps ensure that the system works correctly and that users receive immediate feedback if they enter an invalid command. RPC failing if "start", "status" or "abort" doesn't stop the first command also seems to be a proper way to handle the problem.
Overall, including rigorous unit testing is a best practice in software programming, especi
...
💬 stickies-v commented on pull request "Allow configuring target block time for a signet":
(https://github.com/bitcoin/bitcoin/pull/27446#discussion_r1164480479)
You're right, we do want to throw. `AppInitParameterInteraction()` does seem to raise `InitError` based on bad parameter interaction. I'm not sure why we have both `AppInitParameterInteraction()` and `InitParameterInteraction()` and which needs to be used when, it just seems like since this effectively is parameter interaction we'd like to raise that somewhere in here. Hopefully someone else can chime in?
(https://github.com/bitcoin/bitcoin/pull/27446#discussion_r1164480479)
You're right, we do want to throw. `AppInitParameterInteraction()` does seem to raise `InitError` based on bad parameter interaction. I'm not sure why we have both `AppInitParameterInteraction()` and `InitParameterInteraction()` and which needs to be used when, it just seems like since this effectively is parameter interaction we'd like to raise that somewhere in here. Hopefully someone else can chime in?
🤔 brunoerg reviewed a pull request: "test: cover addrv2 anchors by adding TorV3 to CAddress in messages.py"
(https://github.com/bitcoin/bitcoin/pull/27452#pullrequestreview-1381854522)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/27452#pullrequestreview-1381854522)
Concept ACK
💬 earuak commented on pull request "test: cover addrv2 anchors by adding TorV3 to CAddress in messages.py":
(https://github.com/bitcoin/bitcoin/pull/27452#issuecomment-1505723431)
It is important to include test cases to ensure that the code is working correctly and that adding new features like logic to compute the addrv2 serialization of the onion v3 address can help improve the functionality of Bitcoin. Also, it's always good to keep working on future coverage for other modules and features.
(https://github.com/bitcoin/bitcoin/pull/27452#issuecomment-1505723431)
It is important to include test cases to ensure that the code is working correctly and that adding new features like logic to compute the addrv2 serialization of the onion v3 address can help improve the functionality of Bitcoin. Also, it's always good to keep working on future coverage for other modules and features.
👍 instagibbs approved a pull request: "mempool: disallow txns under min relay fee, even in packages"
(https://github.com/bitcoin/bitcoin/pull/26933#pullrequestreview-1381762702)
ACK 6fb01d26c57f6af7205721e528bbafee8fa41027
(https://github.com/bitcoin/bitcoin/pull/26933#pullrequestreview-1381762702)
ACK 6fb01d26c57f6af7205721e528bbafee8fa41027
💬 instagibbs commented on pull request "mempool: disallow txns under min relay fee, even in packages":
(https://github.com/bitcoin/bitcoin/pull/26933#discussion_r1164440396)
Could we use `CTxMemPool::info` on parent and child to assert that the (fee + nFeeDelta) summed == 0 for the test so I don't have to think so hard to know this is right
(https://github.com/bitcoin/bitcoin/pull/26933#discussion_r1164440396)
Could we use `CTxMemPool::info` on parent and child to assert that the (fee + nFeeDelta) summed == 0 for the test so I don't have to think so hard to know this is right
💬 instagibbs commented on pull request "mempool: disallow txns under min relay fee, even in packages":
(https://github.com/bitcoin/bitcoin/pull/26933#discussion_r1164429604)
name the 200 `low_fee_amt` and reuse everywhere?
(https://github.com/bitcoin/bitcoin/pull/26933#discussion_r1164429604)
name the 200 `low_fee_amt` and reuse everywhere?
💬 instagibbs commented on pull request "mempool: disallow txns under min relay fee, even in packages":
(https://github.com/bitcoin/bitcoin/pull/26933#discussion_r1164445008)
use `parent_fee`
(https://github.com/bitcoin/bitcoin/pull/26933#discussion_r1164445008)
use `parent_fee`
💬 instagibbs commented on pull request "mempool: disallow txns under min relay fee, even in packages":
(https://github.com/bitcoin/bitcoin/pull/26933#discussion_r1164445361)
use `child_fee`
(https://github.com/bitcoin/bitcoin/pull/26933#discussion_r1164445361)
use `child_fee`
💬 MarcoFalke commented on pull request "Allow configuring target block time for a signet":
(https://github.com/bitcoin/bitcoin/pull/27446#discussion_r1164490851)
Any reason to leak implementation details (`__func__`) to the user when it doesn't add any value? `-signetblocktime cannot be set without -signetchallenge` should be self-explanatory?
(https://github.com/bitcoin/bitcoin/pull/27446#discussion_r1164490851)
Any reason to leak implementation details (`__func__`) to the user when it doesn't add any value? `-signetblocktime cannot be set without -signetchallenge` should be self-explanatory?
💬 earuak commented on pull request "Remove BIP35 mempool p2p message":
(https://github.com/bitcoin/bitcoin/pull/27426#issuecomment-1505730664)
Concept ACK
The BIP35 mempool P2P message, as you mentioned, was originally introduced as a way to share the txids in a node's mempool. However, as you also mentioned, it is no longer widely used and has been closed behind the NetPermissionFlags::Mempool flag due to privacy concerns.
Based on this, it seems reasonable to simplify the P2P code by removing this message, as long as this can be done without changing the protocol version. However, before doing so, it is important to ensure t
...
(https://github.com/bitcoin/bitcoin/pull/27426#issuecomment-1505730664)
Concept ACK
The BIP35 mempool P2P message, as you mentioned, was originally introduced as a way to share the txids in a node's mempool. However, as you also mentioned, it is no longer widely used and has been closed behind the NetPermissionFlags::Mempool flag due to privacy concerns.
Based on this, it seems reasonable to simplify the P2P code by removing this message, as long as this can be done without changing the protocol version. However, before doing so, it is important to ensure t
...
👍 pinheadmz approved a pull request: "Fix logging RPC and -debugexclude with 0/none values, add test coverage, improve docs"
(https://github.com/bitcoin/bitcoin/pull/27231#pullrequestreview-1381837068)
ACK e9f6fd0e028e6a5669926926792c70c31a26c403
reviewed code, built and ran tests locally. played with feature in regtest, tried to break it, couldn't break it.
just one non-busting question below
<details><summary>Show Signature</summary>
```
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
ACK e9f6fd0e028e6a5669926926792c70c31a26c403
-----BEGIN PGP SIGNATURE-----
iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmQ29zkACgkQ5+KYS2KJ
yTqh4hAAwBBMTmfeyYhdSTLpO2hma6GolZdUdvzOYdMhADsu
...
(https://github.com/bitcoin/bitcoin/pull/27231#pullrequestreview-1381837068)
ACK e9f6fd0e028e6a5669926926792c70c31a26c403
reviewed code, built and ran tests locally. played with feature in regtest, tried to break it, couldn't break it.
just one non-busting question below
<details><summary>Show Signature</summary>
```
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
ACK e9f6fd0e028e6a5669926926792c70c31a26c403
-----BEGIN PGP SIGNATURE-----
iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmQ29zkACgkQ5+KYS2KJ
yTqh4hAAwBBMTmfeyYhdSTLpO2hma6GolZdUdvzOYdMhADsu
...
💬 pinheadmz commented on pull request "Fix logging RPC and -debugexclude with 0/none values, add test coverage, improve docs":
(https://github.com/bitcoin/bitcoin/pull/27231#discussion_r1164470470)
Could you combine this into one function? You have another method called `EnableOrDisableLogCategories()` in `node.cpp` which might be confusing. And unlike in `node.cpp`, I don't think the `common.cpp` method is called anywhere else besides `SetLoggingCategories()`
(https://github.com/bitcoin/bitcoin/pull/27231#discussion_r1164470470)
Could you combine this into one function? You have another method called `EnableOrDisableLogCategories()` in `node.cpp` which might be confusing. And unlike in `node.cpp`, I don't think the `common.cpp` method is called anywhere else besides `SetLoggingCategories()`
👋 mzumsande's pull request is ready for review: "p2p: Restrict self-advertisements with privacy networks to avoid fingerprinting"
(https://github.com/bitcoin/bitcoin/pull/27411)
(https://github.com/bitcoin/bitcoin/pull/27411)
💬 earuak commented on issue "BIP158: block filter encoding is inefficient in case of hash collisions":
(https://github.com/bitcoin/bitcoin/issues/27003#issuecomment-1505742416)
The problem described in relation to the BIP158 is indeed a real problem. When two different elements map to the same hash value (hash collision), the block filter ends up containing a higher number of false positives than expected.
The most common solution for dealing with hash collisions is to use a hash function with a lower collision probability, such as SHA-256 instead of SHA-512. In addition, insertion of duplicate elements in the block filter can be avoided by adding functionality to r
...
(https://github.com/bitcoin/bitcoin/issues/27003#issuecomment-1505742416)
The problem described in relation to the BIP158 is indeed a real problem. When two different elements map to the same hash value (hash collision), the block filter ends up containing a higher number of false positives than expected.
The most common solution for dealing with hash collisions is to use a hash function with a lower collision probability, such as SHA-256 instead of SHA-512. In addition, insertion of duplicate elements in the block filter can be avoided by adding functionality to r
...