💬 LaurentMT commented on pull request "Reduce minrelaytxfee to 100 sats/kvB":
(https://github.com/bitcoin/bitcoin/pull/32959#issuecomment-3072569428)
> That value is used to calculate the minimum output amount, not the fee rate of a tx. That seems orthogonal to the goal of this PR.
The two concepts of "dust limit" and minRelayTxFee are logically related and the former was originally derived from the latter in the implementation (see commit [eb30d1a](https://github.com/morcos/bitcoin/commit/eb30d1a5b215c6dd3763d7f7948f2dd8cb61f6bf#diff-cc0c6a9039a1c9fe38b8a21fe28391fffbac9b8531dfda0f658919a9f74b46baL695-R698)).
If this logical relationsh
...
(https://github.com/bitcoin/bitcoin/pull/32959#issuecomment-3072569428)
> That value is used to calculate the minimum output amount, not the fee rate of a tx. That seems orthogonal to the goal of this PR.
The two concepts of "dust limit" and minRelayTxFee are logically related and the former was originally derived from the latter in the implementation (see commit [eb30d1a](https://github.com/morcos/bitcoin/commit/eb30d1a5b215c6dd3763d7f7948f2dd8cb61f6bf#diff-cc0c6a9039a1c9fe38b8a21fe28391fffbac9b8531dfda0f658919a9f74b46baL695-R698)).
If this logical relationsh
...
💬 maflcko commented on pull request "test: use notarized v28.2 binaries and fix macOS detection":
(https://github.com/bitcoin/bitcoin/pull/32922#issuecomment-3072572623)
review ACK 4bb4c865999bfa0b0cb5aa806cf62dbf982fcfe9 🚏
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK 4bb4c865999b
...
(https://github.com/bitcoin/bitcoin/pull/32922#issuecomment-3072572623)
review ACK 4bb4c865999bfa0b0cb5aa806cf62dbf982fcfe9 🚏
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK 4bb4c865999b
...
📝 w0xlt opened a pull request: "wallet: Remove `CWallet::nWalletVersion` and several related functions"
(https://github.com/bitcoin/bitcoin/pull/32977)
This PR incorporates the suggestion provided by @PRabahy and @pablomartin4btc in https://github.com/bitcoin/bitcoin/pull/32944 of removing `CWallet::nWalletVersion` and several related functions, such as `SetMinVersion()`, `GetVersion()`, `GetClosestWalletFeature()`, `IsFeatureSupported()`, `CanSupportFeature()`, etc ...
This field is no longer used in the descriptor wallet and there is still a lot of code related to it, so the changes here provide a good cleanup in the wallet code.
Built
...
(https://github.com/bitcoin/bitcoin/pull/32977)
This PR incorporates the suggestion provided by @PRabahy and @pablomartin4btc in https://github.com/bitcoin/bitcoin/pull/32944 of removing `CWallet::nWalletVersion` and several related functions, such as `SetMinVersion()`, `GetVersion()`, `GetClosestWalletFeature()`, `IsFeatureSupported()`, `CanSupportFeature()`, etc ...
This field is no longer used in the descriptor wallet and there is still a lot of code related to it, so the changes here provide a good cleanup in the wallet code.
Built
...
💬 sybot99 commented on issue "GUI bitcoin core shows wrong amount":
(https://github.com/bitcoin/bitcoin/issues/32976#issuecomment-3072650409)
> That is likely a change address, which is created normally. See for example https://bitcoin.stackexchange.com/questions/736/how-does-change-work-in-a-bitcoin-transaction
>
> Usually the issue tracker is used to track technical issues related to the Bitcoin Core code base.
>
> General bitcoin questions and/or support requests are best directed to the [Bitcoin StackExchange](https://bitcoin.stackexchange.com) or the `#bitcoin` IRC channel on Libera Chat, or one of the Bitcoin subreddits, or an
...
(https://github.com/bitcoin/bitcoin/issues/32976#issuecomment-3072650409)
> That is likely a change address, which is created normally. See for example https://bitcoin.stackexchange.com/questions/736/how-does-change-work-in-a-bitcoin-transaction
>
> Usually the issue tracker is used to track technical issues related to the Bitcoin Core code base.
>
> General bitcoin questions and/or support requests are best directed to the [Bitcoin StackExchange](https://bitcoin.stackexchange.com) or the `#bitcoin` IRC channel on Libera Chat, or one of the Bitcoin subreddits, or an
...
💬 w0xlt commented on pull request "wallet: Remove `upgradewallet` RPC":
(https://github.com/bitcoin/bitcoin/pull/32944#discussion_r2206831594)
@Prabhat1308 @pablomartin4btc thanks for the suggestion. Done in https://github.com/bitcoin/bitcoin/pull/32977.
(https://github.com/bitcoin/bitcoin/pull/32944#discussion_r2206831594)
@Prabhat1308 @pablomartin4btc thanks for the suggestion. Done in https://github.com/bitcoin/bitcoin/pull/32977.
📝 fanquake converted_to_draft a pull request: "wallet: Remove `CWallet::nWalletVersion` and several related functions"
(https://github.com/bitcoin/bitcoin/pull/32977)
This PR incorporates the suggestion provided by PRabahy and pablomartin4btc in https://github.com/bitcoin/bitcoin/pull/32944 of removing `CWallet::nWalletVersion` and several related functions, such as `SetMinVersion()`, `GetVersion()`, `GetClosestWalletFeature()`, `IsFeatureSupported()`, `CanSupportFeature()`, etc ...
This field is no longer used in the descriptor wallet and there is still a lot of code related to it, so the changes here provide a good cleanup in the wallet code.
Built on
...
(https://github.com/bitcoin/bitcoin/pull/32977)
This PR incorporates the suggestion provided by PRabahy and pablomartin4btc in https://github.com/bitcoin/bitcoin/pull/32944 of removing `CWallet::nWalletVersion` and several related functions, such as `SetMinVersion()`, `GetVersion()`, `GetClosestWalletFeature()`, `IsFeatureSupported()`, `CanSupportFeature()`, etc ...
This field is no longer used in the descriptor wallet and there is still a lot of code related to it, so the changes here provide a good cleanup in the wallet code.
Built on
...
💬 josibake commented on pull request "Silent Payments: Receiving":
(https://github.com/bitcoin/bitcoin/pull/32966#discussion_r2206887912)
We can disable the fast rescan for now just to keep this PR focused, but in theory we should be able to use the fast rescan regardless of whether or not the wallet is a silent payments wallet. Perhaps we can leave a comment to enable fast rescans in a follow up?
(https://github.com/bitcoin/bitcoin/pull/32966#discussion_r2206887912)
We can disable the fast rescan for now just to keep this PR focused, but in theory we should be able to use the fast rescan regardless of whether or not the wallet is a silent payments wallet. Perhaps we can leave a comment to enable fast rescans in a follow up?
💬 fanquake commented on pull request "rpc,net: Add getpeerbyid and listpeersbyids RPCs":
(https://github.com/bitcoin/bitcoin/pull/32972#issuecomment-3072774251)
Yea, please combine these changes into #32741.
(https://github.com/bitcoin/bitcoin/pull/32972#issuecomment-3072774251)
Yea, please combine these changes into #32741.
✅ fanquake closed a pull request: "rpc,net: Add getpeerbyid and listpeersbyids RPCs"
(https://github.com/bitcoin/bitcoin/pull/32972)
(https://github.com/bitcoin/bitcoin/pull/32972)
💬 sstone commented on pull request "Add a "tx output spender" index":
(https://github.com/bitcoin/bitcoin/pull/24539#issuecomment-3072820488)
> I think that we can simplify collision handling a bit and allow pruning if the index data is stored using the following schema:
>
> * key: `siphash(spent_outpoint) + serialize(tx_disk_position)`
> * value: `b""`
>
> instead of storing a list of positions per `siphash(spent_outpoint)`.
>
> It supports fast lookup of a outpoint spender by using LevelDB prefix scan (similar to how it's done in [electrs](https://github.com/romanz/electrs/blob/master/doc/schema.md) and [bindex](https://do
...
(https://github.com/bitcoin/bitcoin/pull/24539#issuecomment-3072820488)
> I think that we can simplify collision handling a bit and allow pruning if the index data is stored using the following schema:
>
> * key: `siphash(spent_outpoint) + serialize(tx_disk_position)`
> * value: `b""`
>
> instead of storing a list of positions per `siphash(spent_outpoint)`.
>
> It supports fast lookup of a outpoint spender by using LevelDB prefix scan (similar to how it's done in [electrs](https://github.com/romanz/electrs/blob/master/doc/schema.md) and [bindex](https://do
...
💬 maflcko commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2206999295)
nit in 4a8f7fe052d84706cb81c09fb3bc975ce414c7e0: This is removed in the next commit, so it seems better to leave as-is instead of changing/reviewing deleted code. (Same for the scripted diff)
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2206999295)
nit in 4a8f7fe052d84706cb81c09fb3bc975ce414c7e0: This is removed in the next commit, so it seems better to leave as-is instead of changing/reviewing deleted code. (Same for the scripted diff)
💬 maflcko commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2207004439)
nit in 72c775d58a70771efa48acf12da7a6d434adf04e: Could make sense to move this file to `src/util`, to avoid bloating the root source dir. I know this is a stand-alone header right now, but in the future this could also make it easier to move it to the util lib, if there is need.
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2207004439)
nit in 72c775d58a70771efa48acf12da7a6d434adf04e: Could make sense to move this file to `src/util`, to avoid bloating the root source dir. I know this is a stand-alone header right now, but in the future this could also make it easier to move it to the util lib, if there is need.
👍 maflcko approved a pull request: "[IBD] multi-byte block obfuscation"
(https://github.com/bitcoin/bitcoin/pull/31144#pullrequestreview-3019558737)
looked at a6d254e2462cb0d4276e5388b540e02ebc2366c9
(https://github.com/bitcoin/bitcoin/pull/31144#pullrequestreview-3019558737)
looked at a6d254e2462cb0d4276e5388b540e02ebc2366c9
💬 maflcko commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2207018700)
nit in 72c775d58a70771efa48acf12da7a6d434adf04e:
I still don't understand why the same line is changed/renamed several times, when it could just be changed a single time.
it is changed xor_key -> obfuscation -> key_bytes
Also, first hard-coding `8` and then changing all the same lines to `Obfuscation::KEY_SIZE` seems odd. Why not let the very first commit be:
```
class Obfuscation
{
public:
static constexpr size_t KEY_SIZE{sizeof(uint64_t)};
};
```
and then use that fr
...
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2207018700)
nit in 72c775d58a70771efa48acf12da7a6d434adf04e:
I still don't understand why the same line is changed/renamed several times, when it could just be changed a single time.
it is changed xor_key -> obfuscation -> key_bytes
Also, first hard-coding `8` and then changing all the same lines to `Obfuscation::KEY_SIZE` seems odd. Why not let the very first commit be:
```
class Obfuscation
{
public:
static constexpr size_t KEY_SIZE{sizeof(uint64_t)};
};
```
and then use that fr
...
💬 maflcko commented on issue "GUI bitcoin core shows wrong amount":
(https://github.com/bitcoin/bitcoin/issues/32976#issuecomment-3072971242)
You can use the coin selection expert feature, or the RPC interface
(https://github.com/bitcoin/bitcoin/issues/32976#issuecomment-3072971242)
You can use the coin selection expert feature, or the RPC interface
💬 fanquake commented on pull request "cmake: Move internal binaries from bin/ to libexec/":
(https://github.com/bitcoin/bitcoin/pull/31679#issuecomment-3073060232)
@theuni @hebasto @purpleKarrot any opinons/discussion you want to add?
(https://github.com/bitcoin/bitcoin/pull/31679#issuecomment-3073060232)
@theuni @hebasto @purpleKarrot any opinons/discussion you want to add?
✅ fanquake closed an issue: "Intermittent failure in rpc_invalidateblock.py assert_equal(self.nodes[0].getblockchaininfo()['headers'], 7) [ AssertionError: not(24 == 7)]"
(https://github.com/bitcoin/bitcoin/issues/32965)
(https://github.com/bitcoin/bitcoin/issues/32965)
🚀 fanquake merged a pull request: "test: fix intermittent failure in rpc_invalidateblock.py"
(https://github.com/bitcoin/bitcoin/pull/32968)
(https://github.com/bitcoin/bitcoin/pull/32968)
🚀 fanquake merged a pull request: "test: headers sync timeout"
(https://github.com/bitcoin/bitcoin/pull/32677)
(https://github.com/bitcoin/bitcoin/pull/32677)
💬 marcofleon commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#issuecomment-3073205784)
ReACK 50024620b909fc30b68a3715680e963f048482a5
A couple additional assertions, some nits addressed, and improvements in the `txorphanage_sim` fuzz target since last review. Ran the fuzz tests for a bit on existing corpora to be sure.
(https://github.com/bitcoin/bitcoin/pull/31829#issuecomment-3073205784)
ReACK 50024620b909fc30b68a3715680e963f048482a5
A couple additional assertions, some nits addressed, and improvements in the `txorphanage_sim` fuzz target since last review. Ran the fuzz tests for a bit on existing corpora to be sure.