💬 sipa commented on pull request "cluster mempool: add TxGraph work controls":
(https://github.com/bitcoin/bitcoin/pull/32263#issuecomment-3074471173)
Will address the outstanding comments if I touch up.
(https://github.com/bitcoin/bitcoin/pull/32263#issuecomment-3074471173)
Will address the outstanding comments if I touch up.
✅ fanquake closed a pull request: "Bitcoin.png"
(https://github.com/bitcoin/bitcoin/pull/32980)
(https://github.com/bitcoin/bitcoin/pull/32980)
📝 Sazwan96 opened a pull request: "Bitcoin Core"
(https://github.com/bitcoin/bitcoin/pull/32981)
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improv
...
(https://github.com/bitcoin/bitcoin/pull/32981)
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improv
...
💬 Sazwan96 commented on pull request "Bitcoin Core":
(https://github.com/bitcoin/bitcoin/pull/32981#issuecomment-3074498291)
Helo
(https://github.com/bitcoin/bitcoin/pull/32981#issuecomment-3074498291)
Helo
✅ pinheadmz closed a pull request: "Bitcoin Core"
(https://github.com/bitcoin/bitcoin/pull/32981)
(https://github.com/bitcoin/bitcoin/pull/32981)
💬 RobinLinus commented on pull request "Reduce minrelaytxfee to 100 sats/kvB":
(https://github.com/bitcoin/bitcoin/pull/32959#issuecomment-3074619747)
> DUST_RELAY_TX_FEE should be decreased along with this as well...
DEFAULT_MIN_RELAY_TX_FEE and DUST_RELAY_TX_FEE serve different purposes. The former is about protecting the P2P network, while the latter also aims at protecting the UTXO set from DoS attacks. Given that half of the UTXO set is spam these days, it seems like the dust limit should rather get _increased_ than decreased. So it's a whole different discussion, and I would not want to conflate these two in this PR.
(https://github.com/bitcoin/bitcoin/pull/32959#issuecomment-3074619747)
> DUST_RELAY_TX_FEE should be decreased along with this as well...
DEFAULT_MIN_RELAY_TX_FEE and DUST_RELAY_TX_FEE serve different purposes. The former is about protecting the P2P network, while the latter also aims at protecting the UTXO set from DoS attacks. Given that half of the UTXO set is spam these days, it seems like the dust limit should rather get _increased_ than decreased. So it's a whole different discussion, and I would not want to conflate these two in this PR.
💬 pinheadmz commented on pull request "bitcoin-cli: Add -ipcconnect option":
(https://github.com/bitcoin/bitcoin/pull/32297#issuecomment-3074628613)
I got some crashes testing 37cd2c076434e7acbdbb20996cf87afb2cb5bc84 on macos/arm64. This is on a pruned mainnet node catching up on about 30 days behind blockchain tip. After these two crashes I wasn't able to reproduce any more for a little while after a few restarts then got the third crash.
**server:** `build/bin/bitcoin-node -server=1 -printtoconsole=1 -ipcbind=unix -debug=ipc -debug=rpc -debug=http`
**client:** `build/bin/bitcoin-cli -getinfo`
**server:**
```
2025-07-15T17:16:16Z
...
(https://github.com/bitcoin/bitcoin/pull/32297#issuecomment-3074628613)
I got some crashes testing 37cd2c076434e7acbdbb20996cf87afb2cb5bc84 on macos/arm64. This is on a pruned mainnet node catching up on about 30 days behind blockchain tip. After these two crashes I wasn't able to reproduce any more for a little while after a few restarts then got the third crash.
**server:** `build/bin/bitcoin-node -server=1 -printtoconsole=1 -ipcbind=unix -debug=ipc -debug=rpc -debug=http`
**client:** `build/bin/bitcoin-cli -getinfo`
**server:**
```
2025-07-15T17:16:16Z
...
💬 w0xlt commented on pull request "wallet: Remove `CWallet::nWalletVersion` and several related functions":
(https://github.com/bitcoin/bitcoin/pull/32977#discussion_r2208180280)
Done in https://github.com/bitcoin/bitcoin/pull/32977/commits/fc4d563edb5feab273e5770f1b369cb6fe8a0c8c
(https://github.com/bitcoin/bitcoin/pull/32977#discussion_r2208180280)
Done in https://github.com/bitcoin/bitcoin/pull/32977/commits/fc4d563edb5feab273e5770f1b369cb6fe8a0c8c
💬 w0xlt commented on pull request "wallet: Remove `CWallet::nWalletVersion` and several related functions":
(https://github.com/bitcoin/bitcoin/pull/32977#discussion_r2208180979)
Done in https://github.com/bitcoin/bitcoin/commit/fc4d563edb5feab273e5770f1b369cb6fe8a0c8c
(https://github.com/bitcoin/bitcoin/pull/32977#discussion_r2208180979)
Done in https://github.com/bitcoin/bitcoin/commit/fc4d563edb5feab273e5770f1b369cb6fe8a0c8c
💬 w0xlt commented on pull request "wallet: Remove `CWallet::nWalletVersion` and several related functions":
(https://github.com/bitcoin/bitcoin/pull/32977#discussion_r2208183447)
Done in https://github.com/bitcoin/bitcoin/pull/32977/commits/269c43bdb04768954e59119363afdfa061849ed0
(https://github.com/bitcoin/bitcoin/pull/32977#discussion_r2208183447)
Done in https://github.com/bitcoin/bitcoin/pull/32977/commits/269c43bdb04768954e59119363afdfa061849ed0
💬 enirox001 commented on pull request "test: Fix reorg patterns in tests to use proper fork-based approach":
(https://github.com/bitcoin/bitcoin/pull/32587#issuecomment-3074636173)
Code review and tested ACK
This PR looks good to me. The approach to modularize the reorg method and improve the reorg behavior is well thought out and implemented.
- **da836ca**: Move to the `blocktools.py` file done well - this makes it easier to reuse across multiple files
- **3afc731**: Reorg behavior changed in the respective files matches real-world behavior as opposed to the previous implementation
I do have a question regarding some duplication:
(https://github.com/bitcoin/bitcoin/pull/32587#issuecomment-3074636173)
Code review and tested ACK
This PR looks good to me. The approach to modularize the reorg method and improve the reorg behavior is well thought out and implemented.
- **da836ca**: Move to the `blocktools.py` file done well - this makes it easier to reuse across multiple files
- **3afc731**: Reorg behavior changed in the respective files matches real-world behavior as opposed to the previous implementation
I do have a question regarding some duplication:
💬 w0xlt commented on pull request "wallet: Remove `CWallet::nWalletVersion` and several related functions":
(https://github.com/bitcoin/bitcoin/pull/32977#discussion_r2208185081)
Done in https://github.com/bitcoin/bitcoin/pull/32977/commits/8462d84c596315d1979d26b964657079ed5bc09b
(https://github.com/bitcoin/bitcoin/pull/32977#discussion_r2208185081)
Done in https://github.com/bitcoin/bitcoin/pull/32977/commits/8462d84c596315d1979d26b964657079ed5bc09b
💬 w0xlt commented on pull request "wallet: Remove `CWallet::nWalletVersion` and several related functions":
(https://github.com/bitcoin/bitcoin/pull/32977#discussion_r2208190100)
In this case, it needs to be removed. Since the wallet version is no longer loaded from the database file, this record no longer appears in the log.
(https://github.com/bitcoin/bitcoin/pull/32977#discussion_r2208190100)
In this case, it needs to be removed. Since the wallet version is no longer loaded from the database file, this record no longer appears in the log.
💬 w0xlt commented on pull request "wallet: Remove wallet version and several legacy related functions":
(https://github.com/bitcoin/bitcoin/pull/32977#issuecomment-3074653838)
@pablomartin4btc thanks for the detailed review. I've addressed all your comments.
(https://github.com/bitcoin/bitcoin/pull/32977#issuecomment-3074653838)
@pablomartin4btc thanks for the detailed review. I've addressed all your comments.
💬 glozow commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#issuecomment-3074663577)
> Looking at logs, was wondering if we can get some more information about which peer/ which tx is being evicted from the orphanage?
Will add to the followup. What about adding a log for each peer chosen in the loop? So for example 1 call to `LimitOrphans`:
```
[txpackages] peer=25 orphanage overflow, removed 4 announcements
[txpackages] peer=177 orphanage overflow, removed 1 announcements
[txpackages] peer=25 orphanage overflow, removed 1 announcements
[txpackages] orphanage overflow,
...
(https://github.com/bitcoin/bitcoin/pull/31829#issuecomment-3074663577)
> Looking at logs, was wondering if we can get some more information about which peer/ which tx is being evicted from the orphanage?
Will add to the followup. What about adding a log for each peer chosen in the loop? So for example 1 call to `LimitOrphans`:
```
[txpackages] peer=25 orphanage overflow, removed 4 announcements
[txpackages] peer=177 orphanage overflow, removed 1 announcements
[txpackages] peer=25 orphanage overflow, removed 1 announcements
[txpackages] orphanage overflow,
...
💬 Galoretka commented on pull request "fix: Python 3 bytes comparison in linearize-data.py":
(https://github.com/bitcoin/bitcoin/pull/32978#issuecomment-3074833379)
> Are there steps to reproduce, or a test to confirm the fix?
The change is necessary because, in Python 3, indexing a bytes object returns an integer, not a single-character string. For example:
> ```python
> b"\0"[0] == "\0" # always False in Python 3
> b"\0"[0] == 0 # True
> ```
So, the previous comparison would never detect a null byte as intended.
(https://github.com/bitcoin/bitcoin/pull/32978#issuecomment-3074833379)
> Are there steps to reproduce, or a test to confirm the fix?
The change is necessary because, in Python 3, indexing a bytes object returns an integer, not a single-character string. For example:
> ```python
> b"\0"[0] == "\0" # always False in Python 3
> b"\0"[0] == 0 # True
> ```
So, the previous comparison would never detect a null byte as intended.
💬 achow101 commented on pull request "doc: clarify note about backup after migratewallet":
(https://github.com/bitcoin/bitcoin/pull/32956#issuecomment-3074847514)
I don't see why this is needed. There is are already more specific instructions about the backup file in the previous paragraph. The term "backup" already implies that it will not be changed.
(https://github.com/bitcoin/bitcoin/pull/32956#issuecomment-3074847514)
I don't see why this is needed. There is are already more specific instructions about the backup file in the previous paragraph. The term "backup" already implies that it will not be changed.
💬 maflcko commented on pull request "fix: Python 3 bytes comparison in linearize-data.py":
(https://github.com/bitcoin/bitcoin/pull/32978#issuecomment-3074852568)
I was asking about an end-to-end test, like test/functional/feature_loadblock.py or by calling linearize-data.py manually
(https://github.com/bitcoin/bitcoin/pull/32978#issuecomment-3074852568)
I was asking about an end-to-end test, like test/functional/feature_loadblock.py or by calling linearize-data.py manually
✅ maflcko closed a pull request: "doc: clarify note about backup after migratewallet"
(https://github.com/bitcoin/bitcoin/pull/32956)
(https://github.com/bitcoin/bitcoin/pull/32956)
💬 maflcko commented on pull request "doc: clarify note about backup after migratewallet":
(https://github.com/bitcoin/bitcoin/pull/32956#issuecomment-3074939631)
Agree.
Closing for now, but discussion can continue.
(https://github.com/bitcoin/bitcoin/pull/32956#issuecomment-3074939631)
Agree.
Closing for now, but discussion can continue.