💬 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.
💬 achow101 commented on pull request "wallet: Remove wallet version and several legacy related functions":
(https://github.com/bitcoin/bitcoin/pull/32977#discussion_r2208326435)
In dbf8dbf4b1792686be70050c3b19e477769d142b "wallet: Remove `CWallet::nWalletVersion` and related functions"
We still need to write the minversion record for newly created wallets so that they can be opened in older versions with the expected feature set.
(https://github.com/bitcoin/bitcoin/pull/32977#discussion_r2208326435)
In dbf8dbf4b1792686be70050c3b19e477769d142b "wallet: Remove `CWallet::nWalletVersion` and related functions"
We still need to write the minversion record for newly created wallets so that they can be opened in older versions with the expected feature set.
💬 Eunovo commented on pull request "Silent Payments: Receiving":
(https://github.com/bitcoin/bitcoin/pull/32966#issuecomment-3075063066)
Thanks for the reviews @josibake and @Sjors . I am currently travelling, and I will get to them later in the week.
(https://github.com/bitcoin/bitcoin/pull/32966#issuecomment-3075063066)
Thanks for the reviews @josibake and @Sjors . I am currently travelling, and I will get to them later in the week.
📝 puchu opened a pull request: "set min tx fee to 100sat/kvB"
(https://github.com/bitcoin/bitcoin/pull/32982)
as the price of bitcoin rose and the mempool is already filled with 100sat/kvB txs, accept this reality and reflect the change in the current policy/code
(https://github.com/bitcoin/bitcoin/pull/32982)
as the price of bitcoin rose and the mempool is already filled with 100sat/kvB txs, accept this reality and reflect the change in the current policy/code
✅ pinheadmz closed a pull request: "set min tx fee to 100sat/kvB"
(https://github.com/bitcoin/bitcoin/pull/32982)
(https://github.com/bitcoin/bitcoin/pull/32982)
💬 pinheadmz commented on pull request "set min tx fee to 100sat/kvB":
(https://github.com/bitcoin/bitcoin/pull/32982#issuecomment-3075161914)
closing as duplicate of https://github.com/bitcoin/bitcoin/pull/32959
(https://github.com/bitcoin/bitcoin/pull/32982#issuecomment-3075161914)
closing as duplicate of https://github.com/bitcoin/bitcoin/pull/32959
💬 pablomartin4btc commented on pull request "wallet: Remove `upgradewallet` RPC":
(https://github.com/bitcoin/bitcoin/pull/32944#discussion_r2208459894)
nit (only if you have to re-touch): sorry, my bad, missed the "-" (bullet point) in my previous suggestion but I think that will be added during the release process if there are more updates on RPCs.
```suggestion
- `upgradewallet` has been removed. It was unused and only applied to unsupported legacy wallets.
```
(https://github.com/bitcoin/bitcoin/pull/32944#discussion_r2208459894)
nit (only if you have to re-touch): sorry, my bad, missed the "-" (bullet point) in my previous suggestion but I think that will be added during the release process if there are more updates on RPCs.
```suggestion
- `upgradewallet` has been removed. It was unused and only applied to unsupported legacy wallets.
```
👍 pablomartin4btc approved a pull request: "wallet: Remove `upgradewallet` RPC"
(https://github.com/bitcoin/bitcoin/pull/32944#pullrequestreview-3021991698)
ACK d89c6fa4a71810cdb28395d4609632e1b22249b3
(https://github.com/bitcoin/bitcoin/pull/32944#pullrequestreview-3021991698)
ACK d89c6fa4a71810cdb28395d4609632e1b22249b3
💬 pablomartin4btc commented on pull request "wallet: Remove wallet version and several legacy related functions":
(https://github.com/bitcoin/bitcoin/pull/32977#discussion_r2208482177)
Ok, perhaps we can rename the function to `SetInitialVersion`, `SetCompatibleVersion` or similar with a comment/ note about the intention. Is this something that needs to be kept forever or some "deprecation" case?
(https://github.com/bitcoin/bitcoin/pull/32977#discussion_r2208482177)
Ok, perhaps we can rename the function to `SetInitialVersion`, `SetCompatibleVersion` or similar with a comment/ note about the intention. Is this something that needs to be kept forever or some "deprecation" case?
💬 luke-jr commented on pull request "rpc: add optional peer_ids param to filter getpeerinfo":
(https://github.com/bitcoin/bitcoin/pull/32741#issuecomment-3075411770)
I would do peer_id|peer_ids and accept both a single number (most common use) or an array (efficient).
(https://github.com/bitcoin/bitcoin/pull/32741#issuecomment-3075411770)
I would do peer_id|peer_ids and accept both a single number (most common use) or an array (efficient).
💬 w0xlt commented on pull request "wallet: Remove wallet version and several legacy related functions":
(https://github.com/bitcoin/bitcoin/pull/32977#discussion_r2208578726)
I had changed it to `WriteLatestLegacyWalletVersion` before seeing your comment.
If that doesn't work, we can change it to something else.
(https://github.com/bitcoin/bitcoin/pull/32977#discussion_r2208578726)
I had changed it to `WriteLatestLegacyWalletVersion` before seeing your comment.
If that doesn't work, we can change it to something else.
💬 lifofifoX commented on pull request "Reduce minrelaytxfee to 100 sats/kvB":
(https://github.com/bitcoin/bitcoin/pull/32959#issuecomment-3075441263)
It's ridiculous to obsess over "spam" and have it dictate policy, rather than rely on economic incentives. Dust limits should be set based on it being economical to spend a UTXO at minimum relay fees, rather than your misplaced notion of "spam".
(https://github.com/bitcoin/bitcoin/pull/32959#issuecomment-3075441263)
It's ridiculous to obsess over "spam" and have it dictate policy, rather than rely on economic incentives. Dust limits should be set based on it being economical to spend a UTXO at minimum relay fees, rather than your misplaced notion of "spam".
💬 ishaanam commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2208593588)
Done
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2208593588)
Done
💬 ishaanam commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2208595036)
Done
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2208595036)
Done
💬 yuvicc commented on pull request "test: Fix reorg patterns in tests to use proper fork-based approach":
(https://github.com/bitcoin/bitcoin/pull/32587#issuecomment-3075479833)
> Across several test files, `trigger_reorg` is defined twice:
>
I am not sure if you have reviewed the code nicely, there isn't any duplication but rather a different approach using time-based timelock, see [comment](https://github.com/bitcoin/bitcoin/pull/32587#discussion_r2180262495)
>
> Only the node lookup differs. Would it make sense to extract a single trigger_reorg(fork_blocks, node) utility in test_framework/blocktools.py and have all tests call that?
For now, I think we ca
...
(https://github.com/bitcoin/bitcoin/pull/32587#issuecomment-3075479833)
> Across several test files, `trigger_reorg` is defined twice:
>
I am not sure if you have reviewed the code nicely, there isn't any duplication but rather a different approach using time-based timelock, see [comment](https://github.com/bitcoin/bitcoin/pull/32587#discussion_r2180262495)
>
> Only the node lookup differs. Would it make sense to extract a single trigger_reorg(fork_blocks, node) utility in test_framework/blocktools.py and have all tests call that?
For now, I think we ca
...