π¬ mzumsande commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1445399806)
here, and in `peer_accept_connection`: I think the default values aren't necessary and could be dropped. They are not used right now, and it seems better if future callers are forced make a decision on whether to use v2 or not!
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1445399806)
here, and in `peer_accept_connection`: I think the default values aren't necessary and could be dropped. They are not used right now, and it seems better if future callers are forced make a decision on whether to use v2 or not!
π¬ mzumsande commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1447657783)
Trying to wrap my head around this:
Let's say we receive the garbage and then receive one or more decoy packages. Then we cut the response here, but if no version package has been received so far, we can still return `0, True` earlier in the while loop and try again later.
But then the next time this function is called, wouldn't authentication fail because we've already removed the garbage terminator from the response?
Not a big deal though - because bitcoind doesn't send any decoy message
...
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1447657783)
Trying to wrap my head around this:
Let's say we receive the garbage and then receive one or more decoy packages. Then we cut the response here, but if no version package has been received so far, we can still return `0, True` earlier in the while loop and try again later.
But then the next time this function is called, wouldn't authentication fail because we've already removed the garbage terminator from the response?
Not a big deal though - because bitcoind doesn't send any decoy message
...
π¬ mzumsande commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1447962448)
I think that this should be moved outside of the `if connection_type == "feeler":` clause and be done before:
After all the v2 reconnection logically stuff comes first, and then all the existing v1 logic still applies to the downgraded v1 connection - for example, the reconnection logic should also work if `add_outbound_p2p_connection` specified a feeler connection - in which case we currently wouldn't call `wait_for_reconnect`.
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1447962448)
I think that this should be moved outside of the `if connection_type == "feeler":` clause and be done before:
After all the v2 reconnection logically stuff comes first, and then all the existing v1 logic still applies to the downgraded v1 connection - for example, the reconnection logic should also work if `add_outbound_p2p_connection` specified a feeler connection - in which case we currently wouldn't call `wait_for_reconnect`.
π¬ mzumsande commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1447884179)
nit: `reject` sounds like we actively do something, `ignore` seems better.
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1447884179)
nit: `reject` sounds like we actively do something, `ignore` seems better.
π¬ Eunovo commented on pull request "test: wallet rescan with reorged parent + IsFromMe child in mempool":
(https://github.com/bitcoin/bitcoin/pull/29179#issuecomment-1885834450)
Tested ACK [d9df438](https://github.com/bitcoin/bitcoin/pull/29179/commits/d9df438c6e581dae0c818a4c2f5fe95627ae26bc)
Ran the tests locally and got `test_framework.authproxy.JSONRPCException: Invalid or non-wallet transaction id (-5)` for both `wallet_import_rescan` and `wallet_rescan_unconfirmed` when random order is used in `CTxMemPool::entryAll()`.
(https://github.com/bitcoin/bitcoin/pull/29179#issuecomment-1885834450)
Tested ACK [d9df438](https://github.com/bitcoin/bitcoin/pull/29179/commits/d9df438c6e581dae0c818a4c2f5fe95627ae26bc)
Ran the tests locally and got `test_framework.authproxy.JSONRPCException: Invalid or non-wallet transaction id (-5)` for both `wallet_import_rescan` and `wallet_rescan_unconfirmed` when random order is used in `CTxMemPool::entryAll()`.
π kevkevinpal opened a pull request: "doc: upgrade Bitcoin Core license to 2024"
(https://github.com/bitcoin/bitcoin/pull/29222)
See https://github.com/bitcoin/bitcoin/pull/26748, [23945](https://github.com/bitcoin/bitcoin/pull/23945), https://github.com/bitcoin/bitcoin/pull/20805, https://github.com/bitcoin/bitcoin/pull/17801, https://github.com/bitcoin/bitcoin/pull/15061
Every year had his upgrade :)
---
cherry-picked these commits from this PR by @22388o and then squashed them to open this PR above is the original desc of the PR
https://github.com/kevkevinpal/bitcoin/pull/new/updateBitcoinLicense2024
85a9
...
(https://github.com/bitcoin/bitcoin/pull/29222)
See https://github.com/bitcoin/bitcoin/pull/26748, [23945](https://github.com/bitcoin/bitcoin/pull/23945), https://github.com/bitcoin/bitcoin/pull/20805, https://github.com/bitcoin/bitcoin/pull/17801, https://github.com/bitcoin/bitcoin/pull/15061
Every year had his upgrade :)
---
cherry-picked these commits from this PR by @22388o and then squashed them to open this PR above is the original desc of the PR
https://github.com/kevkevinpal/bitcoin/pull/new/updateBitcoinLicense2024
85a9
...
π¬ mzumsande commented on pull request "Fix -netinfo backward compat with getpeerinfo pre-v26":
(https://github.com/bitcoin/bitcoin/pull/29212#discussion_r1448067295)
ok, if it's on purpose it's fine.
(https://github.com/bitcoin/bitcoin/pull/29212#discussion_r1448067295)
ok, if it's on purpose it's fine.
π¬ mzumsande commented on pull request "Fix -netinfo backward compat with getpeerinfo pre-v26":
(https://github.com/bitcoin/bitcoin/pull/29212#issuecomment-1885855351)
Code Review ACK 5fa74609b833643334dfb5519f2023119984267b
(https://github.com/bitcoin/bitcoin/pull/29212#issuecomment-1885855351)
Code Review ACK 5fa74609b833643334dfb5519f2023119984267b
π¬ mzumsande commented on issue "Witness scripts being abused to bypass datacarriersize limit (CVE-2023-50428)":
(https://github.com/bitcoin/bitcoin/issues/29187#issuecomment-1885891577)
> A softfork making data storage impractical. [...] It would also be a slow process
It's going to be a slow process either way:
Even if Bitcoin Core changed its default policy, the effect on propagation would be negligible for a long time.
The p2p network is densely connected, with reachable nodes having 8 tx-relaying outbound peers and typically many more tx-relaying inbounds. Some network simulations I did in the context of full-RBF a few years ago indicated that already a minority of 1
...
(https://github.com/bitcoin/bitcoin/issues/29187#issuecomment-1885891577)
> A softfork making data storage impractical. [...] It would also be a slow process
It's going to be a slow process either way:
Even if Bitcoin Core changed its default policy, the effect on propagation would be negligible for a long time.
The p2p network is densely connected, with reachable nodes having 8 tx-relaying outbound peers and typically many more tx-relaying inbounds. Some network simulations I did in the context of full-RBF a few years ago indicated that already a minority of 1
...
π¬ murchandamus commented on pull request "Add Gutter Guard Selector":
(https://github.com/bitcoin/bitcoin/pull/28977#issuecomment-1885907384)
Yeah, oldest first might be better for soaking up tiny UTXOs at low feerates that have been piling up during bouts of high feerates.
@achow101: What do you think about one of the two like the following progression
| | <2s/vB | <3s/vB | <4s/vB | <5s/vB | <6s/vB | <7s/vB | <8s/vB | <9s/vB | <10s/vB | >=10s/vB |
| --- | --- | -- | -- | -- | -- | -- | -- | -- | -- | -- |
| [A:] UTXO Limit | +21<sup>β </sup> | +18<sup>β </sup> | +15 | +12 | +10 | +8 | +6 | +5 | +4 |+3|
| [B:] UTXO Limit | +4
...
(https://github.com/bitcoin/bitcoin/pull/28977#issuecomment-1885907384)
Yeah, oldest first might be better for soaking up tiny UTXOs at low feerates that have been piling up during bouts of high feerates.
@achow101: What do you think about one of the two like the following progression
| | <2s/vB | <3s/vB | <4s/vB | <5s/vB | <6s/vB | <7s/vB | <8s/vB | <9s/vB | <10s/vB | >=10s/vB |
| --- | --- | -- | -- | -- | -- | -- | -- | -- | -- | -- |
| [A:] UTXO Limit | +21<sup>β </sup> | +18<sup>β </sup> | +15 | +12 | +10 | +8 | +6 | +5 | +4 |+3|
| [B:] UTXO Limit | +4
...
π¬ andrewtoth commented on pull request "rpc,rest,zmq: faster getblock, NotifyBlock and rest_block by reading raw block":
(https://github.com/bitcoin/bitcoin/pull/26415#discussion_r1448161451)
Fixed. Apparently `IsBlockPruned` is not sufficient to know whether we can read the block or not. Getting the file position of a block that we don't have returns us a null `FlatFilePos`, and `ReadBlockFromDisk` fails on opening a file at null position. `ReadRawBlockFromDisk` instead first tries to decrement `nPos` by 8 before trying to open the file, which results in UB due to `nPos` being unsigned and 0 for a null position.
(https://github.com/bitcoin/bitcoin/pull/26415#discussion_r1448161451)
Fixed. Apparently `IsBlockPruned` is not sufficient to know whether we can read the block or not. Getting the file position of a block that we don't have returns us a null `FlatFilePos`, and `ReadBlockFromDisk` fails on opening a file at null position. `ReadRawBlockFromDisk` instead first tries to decrement `nPos` by 8 before trying to open the file, which results in UB due to `nPos` being unsigned and 0 for a null position.
π¬ ariard commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#issuecomment-1886085872)
I think this is correct to say that nversion=3 is a weak pinning mitigation.
One can always use a revoked state with max outgoing HTLC outputs=483 to pin.
So 483 * 33 + 1000, you have ~18k vbytes state, mempool backlog at 20 sat/vb, you can steal ~100 USD payments.
Yes `max_offered_htlc` chan policy param can be introduced at the LN-level to bound more commitment size.
However mempool backlog will always make the level of pin exposure dynamic, a bad thing.
(Let's be nice and not introduce c
...
(https://github.com/bitcoin/bitcoin/pull/28948#issuecomment-1886085872)
I think this is correct to say that nversion=3 is a weak pinning mitigation.
One can always use a revoked state with max outgoing HTLC outputs=483 to pin.
So 483 * 33 + 1000, you have ~18k vbytes state, mempool backlog at 20 sat/vb, you can steal ~100 USD payments.
Yes `max_offered_htlc` chan policy param can be introduced at the LN-level to bound more commitment size.
However mempool backlog will always make the level of pin exposure dynamic, a bad thing.
(Let's be nice and not introduce c
...
π¬ murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1448210370)
```suggestion
* reach the target. We continue with the direct successor of our `next_utxo` for the
```
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1448210370)
```suggestion
* reach the target. We continue with the direct successor of our `next_utxo` for the
```
π¬ murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1448211928)
```suggestion
// Success, adding more weight cannot be better: SHIFT
```
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1448211928)
```suggestion
// Success, adding more weight cannot be better: SHIFT
```
π¬ saltduck commented on issue "Witness scripts being abused to bypass datacarriersize limit (CVE-2023-50428)":
(https://github.com/bitcoin/bitcoin/issues/29187#issuecomment-1886210780)
Can we just remove the discount of the segwit data? It is fair for both generic users and the attackers.ε¨ 2024εΉ΄1ζ11ζ₯οΌ07:11οΌMartin Zumsande ***@***.***> ειοΌο»Ώ
A softfork making data storage impractical. [...] It would also be a slow process
It's going to be a slow process either way:
Even if Bitcoin Core changed its default policy, the effect on propagation would be negligible for a long time.
The p2p network is densely connected, with reachable nodes having 8 tx-relaying outbound peers an
...
(https://github.com/bitcoin/bitcoin/issues/29187#issuecomment-1886210780)
Can we just remove the discount of the segwit data? It is fair for both generic users and the attackers.ε¨ 2024εΉ΄1ζ11ζ₯οΌ07:11οΌMartin Zumsande ***@***.***> ειοΌο»Ώ
A softfork making data storage impractical. [...] It would also be a slow process
It's going to be a slow process either way:
Even if Bitcoin Core changed its default policy, the effect on propagation would be negligible for a long time.
The p2p network is densely connected, with reachable nodes having 8 tx-relaying outbound peers an
...
π¬ maflcko commented on pull request "doc: upgrade Bitcoin Core license to 2024":
(https://github.com/bitcoin/bitcoin/pull/29222#issuecomment-1886617839)
In the OP: Just linking to the previous year's pull should be enough. Also, the link to open a pull request doesn't make sense. Also, the @-mention should be removed from the pull description, as described in the docs.
(https://github.com/bitcoin/bitcoin/pull/29222#issuecomment-1886617839)
In the OP: Just linking to the previous year's pull should be enough. Also, the link to open a pull request doesn't make sense. Also, the @-mention should be removed from the pull description, as described in the docs.
π€ glozow reviewed a pull request: "mempool / rpc: followup to getprioritisedtransactions and delete a mapDeltas entry when delta==0"
(https://github.com/bitcoin/bitcoin/pull/28885#pullrequestreview-1815052118)
ACK 5353af1c8a2dd402a303a786d8aae5edbfc1ff3a
(https://github.com/bitcoin/bitcoin/pull/28885#pullrequestreview-1815052118)
ACK 5353af1c8a2dd402a303a786d8aae5edbfc1ff3a
π¬ glozow commented on pull request "mempool / rpc: followup to getprioritisedtransactions and delete a mapDeltas entry when delta==0":
(https://github.com/bitcoin/bitcoin/pull/28885#discussion_r1448536720)
dc189d3ec5c4af9bca02928dfeb4770570060982: I'm not sure that "new fee" would be understood by people. It would also be helpful to add the units here, i.e.
```suggestion
{RPCResult::Type::NUM, "modified_fee", /*optional=*/true, "modified fee in satoshis. Only returned if in_mempool=true"},
```
(https://github.com/bitcoin/bitcoin/pull/28885#discussion_r1448536720)
dc189d3ec5c4af9bca02928dfeb4770570060982: I'm not sure that "new fee" would be understood by people. It would also be helpful to add the units here, i.e.
```suggestion
{RPCResult::Type::NUM, "modified_fee", /*optional=*/true, "modified fee in satoshis. Only returned if in_mempool=true"},
```
π BrandonOdiwuor opened a pull request: "wallet: Refactor DumpWallet function to accept -dumpfile path argument"
(https://github.com/bitcoin/bitcoin/pull/29223)
Quick follow-up to https://github.com/bitcoin/bitcoin/pull/29117, coming from _https://github.com/bitcoin/bitcoin/pull/29117#discussion_r1431999422_.
Refactor `DumpWallet(...)` to accept the `-dumpfile` path arg instead of the entire `ArgsManager`
(https://github.com/bitcoin/bitcoin/pull/29223)
Quick follow-up to https://github.com/bitcoin/bitcoin/pull/29117, coming from _https://github.com/bitcoin/bitcoin/pull/29117#discussion_r1431999422_.
Refactor `DumpWallet(...)` to accept the `-dumpfile` path arg instead of the entire `ArgsManager`
π¬ maflcko commented on pull request "wallet: Refactor DumpWallet function to accept -dumpfile path argument":
(https://github.com/bitcoin/bitcoin/pull/29223#issuecomment-1886719702)
Not sure. The current version seems just fine
(https://github.com/bitcoin/bitcoin/pull/29223#issuecomment-1886719702)
Not sure. The current version seems just fine