🤔 mzumsande reviewed a pull request: "net: enable v2transport by default"
(https://github.com/bitcoin/bitcoin/pull/29347#pullrequestreview-1851551229)
Concept ACK
> @willcl-ark I believe @mzumsande is planning to open a PR to enable it everywhere in the tests.
Yes, I'll open a PR later this week (current branch https://github.com/mzumsande/bitcoin/tree/202401_bip324_alltests which still needs some cleanups)!
> If we are now defaulting v2 to enabled, do we want to enable v2transport on a few more of the p2p functional tests
It's more than adding more tests to the list, the `-v2transport` option currently doesn't make the python `P2P
...
(https://github.com/bitcoin/bitcoin/pull/29347#pullrequestreview-1851551229)
Concept ACK
> @willcl-ark I believe @mzumsande is planning to open a PR to enable it everywhere in the tests.
Yes, I'll open a PR later this week (current branch https://github.com/mzumsande/bitcoin/tree/202401_bip324_alltests which still needs some cleanups)!
> If we are now defaulting v2 to enabled, do we want to enable v2transport on a few more of the p2p functional tests
It's more than adding more tests to the list, the `-v2transport` option currently doesn't make the python `P2P
...
💬 stratospher commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#issuecomment-1917230742)
hmm weird, looking into it.
i also have a [WIP branch](https://github.com/stratospher/bitcoin/commits/more-v2-tests/) which uses this test file and adds more v2 tests for sending excess garbage bytes, wrong garbage terminator, incorrect ellswift and non-empty version packet.
(https://github.com/bitcoin/bitcoin/pull/24748#issuecomment-1917230742)
hmm weird, looking into it.
i also have a [WIP branch](https://github.com/stratospher/bitcoin/commits/more-v2-tests/) which uses this test file and adds more v2 tests for sending excess garbage bytes, wrong garbage terminator, incorrect ellswift and non-empty version packet.
💬 stratospher commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1471455135)
true, will clean it up in [this branch](https://github.com/stratospher/bitcoin/commits/more-v2-tests/).
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1471455135)
true, will clean it up in [this branch](https://github.com/stratospher/bitcoin/commits/more-v2-tests/).
💬 virtu commented on issue "ASN-based bucketing of the network nodes":
(https://github.com/bitcoin/bitcoin/issues/16599#issuecomment-1917362538)
Seeing some of my previous work already got linked, I wanted to share an updated view on one major and one minor concern I came across during my research on ASMAP.
The major concern relates to second- and third-order effects of establishing only one outbound per AS that lead to several negative outcomes for the P2P network graph. To demonstrate the effects, consider a clearnet node opening ten outbound connections: given that Hetzner's AS comprises around 1k out of a total of 8k reachable cl
...
(https://github.com/bitcoin/bitcoin/issues/16599#issuecomment-1917362538)
Seeing some of my previous work already got linked, I wanted to share an updated view on one major and one minor concern I came across during my research on ASMAP.
The major concern relates to second- and third-order effects of establishing only one outbound per AS that lead to several negative outcomes for the P2P network graph. To demonstrate the effects, consider a clearnet node opening ten outbound connections: given that Hetzner's AS comprises around 1k out of a total of 8k reachable cl
...
💬 theStack commented on pull request "rpc: Do not wait for headers inside loadtxoutset":
(https://github.com/bitcoin/bitcoin/pull/29345#issuecomment-1917386986)
Concept ACK
I tend to agree that it's best to remove the waiting loop and have instant feedback about missing headers instead, as I think that avoids frustration and/or confusion for users. Curious about reasonable arguments to keep it though (maybe I'm missing something).
(https://github.com/bitcoin/bitcoin/pull/29345#issuecomment-1917386986)
Concept ACK
I tend to agree that it's best to remove the waiting loop and have instant feedback about missing headers instead, as I think that avoids frustration and/or confusion for users. Curious about reasonable arguments to keep it though (maybe I'm missing something).
💬 maflcko commented on pull request "assumeutxo, rpc: Add 'start' parameter to loadtxoutset":
(https://github.com/bitcoin/bitcoin/pull/28659#issuecomment-1917432586)
Are you still working on this?
(https://github.com/bitcoin/bitcoin/pull/28659#issuecomment-1917432586)
Are you still working on this?
💬 hebasto commented on issue "The `streams_tests/xor_file` test fails on Windows":
(https://github.com/bitcoin/bitcoin/issues/29014#issuecomment-1917433706)
> Is this fixed in a later version of gcc?
After cross-compiling on Ubuntu 23.10 the issue still remains.
```
$ x86_64-w64-mingw32-g++-posix --version
x86_64-w64-mingw32-g++-posix (GCC) 12-posix
Copyright (C) 2022 Free Software Foundation, Inc.
This is free software; see the source for copying conditions. There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
````
(https://github.com/bitcoin/bitcoin/issues/29014#issuecomment-1917433706)
> Is this fixed in a later version of gcc?
After cross-compiling on Ubuntu 23.10 the issue still remains.
```
$ x86_64-w64-mingw32-g++-posix --version
x86_64-w64-mingw32-g++-posix (GCC) 12-posix
Copyright (C) 2022 Free Software Foundation, Inc.
This is free software; see the source for copying conditions. There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
````
💬 theuni commented on pull request "RFC: Deprecate libconsensus":
(https://github.com/bitcoin/bitcoin/pull/29189#issuecomment-1917436655)
@achow101/ @ajtowns Looking again, I agree the order of operations here probably looks a little strange.
For context, most of this discussion came out of the discussion about porting this to CMake. We went in circles for months trying to decide how to best port this over to CMake, but we eventually got together and asked ourselves: should we even bother? Does anyone actually use this?
This PR was our way of fact-finding. It's a pretty resounding "no". The lib has been around since 2014 an
...
(https://github.com/bitcoin/bitcoin/pull/29189#issuecomment-1917436655)
@achow101/ @ajtowns Looking again, I agree the order of operations here probably looks a little strange.
For context, most of this discussion came out of the discussion about porting this to CMake. We went in circles for months trying to decide how to best port this over to CMake, but we eventually got together and asked ourselves: should we even bother? Does anyone actually use this?
This PR was our way of fact-finding. It's a pretty resounding "no". The lib has been around since 2014 an
...
💬 glozow commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1337024969)
Question: What's the expected state of a transaction if its parent is replaced? My (shallow) understanding suggests it becomes inactive? And if the parent is re-added to mempool, it stays inactive?
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1337024969)
Question: What's the expected state of a transaction if its parent is replaced? My (shallow) understanding suggests it becomes inactive? And if the parent is re-added to mempool, it stays inactive?
🤔 glozow reviewed a pull request: "wallet: track mempool conflicts with wallet transactions"
(https://github.com/bitcoin/bitcoin/pull/27307#pullrequestreview-1643959917)
Approach ACK
(https://github.com/bitcoin/bitcoin/pull/27307#pullrequestreview-1643959917)
Approach ACK
💬 glozow commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1471392482)
in a6ae5b23b1497ab6f4899db49348db623700a2d8
same, `auto` or `Txid` would be better
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1471392482)
in a6ae5b23b1497ab6f4899db49348db623700a2d8
same, `auto` or `Txid` would be better
💬 glozow commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1471531989)
in 067189eddee1a2209c6c42741a91601c5514e170
Got a little nervous seeing the txid comparison and "same tx." Is it possible to run into a same-txid-different-witness situation here?
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1471531989)
in 067189eddee1a2209c6c42741a91601c5514e170
Got a little nervous seeing the txid comparison and "same tx." Is it possible to run into a same-txid-different-witness situation here?
💬 glozow commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1471533005)
c71736d372c65b0978671501029e202574c6bbef
Also good to document - is it just directly conflicting, or descendants as well?
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1471533005)
c71736d372c65b0978671501029e202574c6bbef
Also good to document - is it just directly conflicting, or descendants as well?
💬 glozow commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1471530256)
fwiw, it seems better to keep it the way it was if it's not doing anything. What was the bug?
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1471530256)
fwiw, it seems better to keep it the way it was if it's not doing anything. What was the bug?
💬 glozow commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1471567921)
71059cf4267d80d2934b85a7046fc7b64900378b
There's not anything preventing us from checking the exact contents of `listunspent` right? Here and in the rest of the test.
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1471567921)
71059cf4267d80d2934b85a7046fc7b64900378b
There's not anything preventing us from checking the exact contents of `listunspent` right? Here and in the rest of the test.
💬 glozow commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1471581755)
Not sure if in scope for this PR, but it would be nice to have a test for how the wallet sees descendants of conflicting transactions (e.g. if Bob creates a child off of tx1 or tx2).
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1471581755)
Not sure if in scope for this PR, but it would be nice to have a test for how the wallet sees descendants of conflicting transactions (e.g. if Bob creates a child off of tx1 or tx2).
💬 glozow commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1471391179)
in a6ae5b23b1497ab6f4899db49348db623700a2d8:
It's best to use `Txid` or `Wtxid` for this. And maybe add a doc stating whether these are direct conflicts only, or if they might be descendants of a conflicting transaction?
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1471391179)
in a6ae5b23b1497ab6f4899db49348db623700a2d8:
It's best to use `Txid` or `Wtxid` for this. And maybe add a doc stating whether these are direct conflicts only, or if they might be descendants of a conflicting transaction?
💬 glozow commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1471575174)
71059cf4267d80d2934b85a7046fc7b64900378b
We don't expect to see tx1 in mempool, right? It's not being rebroadcast, as it's inactive?
```suggestion
assert tx1_txid not in self.nodes[0].getrawmempool()
```
Might be good to have a check for this behavior.
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1471575174)
71059cf4267d80d2934b85a7046fc7b64900378b
We don't expect to see tx1 in mempool, right? It's not being rebroadcast, as it's inactive?
```suggestion
assert tx1_txid not in self.nodes[0].getrawmempool()
```
Might be good to have a check for this behavior.
👍 pablomartin4btc approved a pull request: "net: enable v2transport by default"
(https://github.com/bitcoin/bitcoin/pull/29347#pullrequestreview-1851832717)
re ACK 0bef1042ce6c459acb1de965cbccd98867a417f1
(https://github.com/bitcoin/bitcoin/pull/29347#pullrequestreview-1851832717)
re ACK 0bef1042ce6c459acb1de965cbccd98867a417f1
💬 usuarrio-id81214293 commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1471596662)
> Fixed
Need enter wallet
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1471596662)
> Fixed
Need enter wallet