💬 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
💬 ajtowns commented on pull request "RFC: Deprecate libconsensus":
(https://github.com/bitcoin/bitcoin/pull/29189#issuecomment-1917479122)
> Rather than spending time arguing about removing some silly tests, I think there's a compromise here: Let's do the deprecation now (the readme changes as well as a release note as suggested by @achow101 above) and plan to just do the lib removal first thing after branch-off. That way we have our assurances that we can simply skip the port on the CMake side so it won't slow anything down there.
>
> Sound good?
Yeah, that's exactly what I was expecting fwiw.
(https://github.com/bitcoin/bitcoin/pull/29189#issuecomment-1917479122)
> Rather than spending time arguing about removing some silly tests, I think there's a compromise here: Let's do the deprecation now (the readme changes as well as a release note as suggested by @achow101 above) and plan to just do the lib removal first thing after branch-off. That way we have our assurances that we can simply skip the port on the CMake side so it won't slow anything down there.
>
> Sound good?
Yeah, that's exactly what I was expecting fwiw.
💬 hebasto commented on issue "The `streams_tests/xor_file` test fails on Windows":
(https://github.com/bitcoin/bitcoin/issues/29014#issuecomment-1917493926)
The same error when using Ubuntu 24.04:
```
$ x86_64-w64-mingw32-g++-posix --version
x86_64-w64-mingw32-g++-posix (GCC) 13-posix
Copyright (C) 2023 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-1917493926)
The same error when using Ubuntu 24.04:
```
$ x86_64-w64-mingw32-g++-posix --version
x86_64-w64-mingw32-g++-posix (GCC) 13-posix
Copyright (C) 2023 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.
```
💬 hernanmarino commented on pull request "assumeutxo, rpc: Add 'start' parameter to loadtxoutset":
(https://github.com/bitcoin/bitcoin/pull/28659#issuecomment-1917507004)
> Are you still working on this?
Yes ! I was witing for more reviews, but I'll go ahead with @Sjors suggestion asap.
(https://github.com/bitcoin/bitcoin/pull/28659#issuecomment-1917507004)
> Are you still working on this?
Yes ! I was witing for more reviews, but I'll go ahead with @Sjors suggestion asap.
📝 maflcko opened a pull request: " refactor: Remove nChainTx from consensus "
(https://github.com/bitcoin/bitcoin/pull/29349)
`nChainTx` is returned by RPCs and is used to estimate (sync and rescan) progress.
However, it is also used for consensus and P2P logic to denote that a block and all its parents have `BLOCK_VALID_TRANSACTIONS` set.
This is confusing, because:
* It would be clearer to use the `BlockStatus` enum for this as well.
* It requires AssumeUtxo to fake the `nChainTx` values.
* It makes it harder to remove `nChainTx` completely.
Fix it by introducing a new `BLOCK_VALID_TRANSACTIONS_TREE` le
...
(https://github.com/bitcoin/bitcoin/pull/29349)
`nChainTx` is returned by RPCs and is used to estimate (sync and rescan) progress.
However, it is also used for consensus and P2P logic to denote that a block and all its parents have `BLOCK_VALID_TRANSACTIONS` set.
This is confusing, because:
* It would be clearer to use the `BlockStatus` enum for this as well.
* It requires AssumeUtxo to fake the `nChainTx` values.
* It makes it harder to remove `nChainTx` completely.
Fix it by introducing a new `BLOCK_VALID_TRANSACTIONS_TREE` le
...
💬 maflcko commented on issue "The `streams_tests/xor_file` test fails on Windows":
(https://github.com/bitcoin/bitcoin/issues/29014#issuecomment-1917512287)
Ok, as a next step it could make sense to create a standalone reproducer, so that the bug can be reported upstream.
(https://github.com/bitcoin/bitcoin/issues/29014#issuecomment-1917512287)
Ok, as a next step it could make sense to create a standalone reproducer, so that the bug can be reported upstream.
💬 ishaanam commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1471649978)
With this PR, if a transaction's parent is replaced, then it should become `TxStateMempoolConflicted`. If the conflict is removed, then it becomes inactive.
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1471649978)
With this PR, if a transaction's parent is replaced, then it should become `TxStateMempoolConflicted`. If the conflict is removed, then it becomes inactive.
💬 achow101 commented on pull request "consensus: Store transaction nVersion as uint32_t":
(https://github.com/bitcoin/bitcoin/pull/29325#discussion_r1471652116)
Ah, didn't realize we had that. Done
(https://github.com/bitcoin/bitcoin/pull/29325#discussion_r1471652116)
Ah, didn't realize we had that. Done