💬 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
💬 achow101 commented on pull request "consensus: Store transaction nVersion as uint32_t":
(https://github.com/bitcoin/bitcoin/pull/29325#discussion_r1471652269)
Done
(https://github.com/bitcoin/bitcoin/pull/29325#discussion_r1471652269)
Done
💬 ryanofsky commented on issue "assumeutxo: nTx and nChainTx violations in CheckBlockIndex":
(https://github.com/bitcoin/bitcoin/issues/29261#issuecomment-1917533420)
> Let me know if I should reopen this issue?
It's good to close. Extending the assert with https://github.com/bitcoin/bitcoin/issues/29261#issuecomment-1908513362 would only be a marginal improvement. I think a better long term fix would be to get rid of fake `nTx` and `nChainTx` values as described in https://github.com/bitcoin/bitcoin/issues/29328#issuecomment-1914922108 "In the long run...".
(https://github.com/bitcoin/bitcoin/issues/29261#issuecomment-1917533420)
> Let me know if I should reopen this issue?
It's good to close. Extending the assert with https://github.com/bitcoin/bitcoin/issues/29261#issuecomment-1908513362 would only be a marginal improvement. I think a better long term fix would be to get rid of fake `nTx` and `nChainTx` values as described in https://github.com/bitcoin/bitcoin/issues/29328#issuecomment-1914922108 "In the long run...".
💬 maflcko commented on issue "assumeutxo: nTx and nChainTx in RPC results are off":
(https://github.com/bitcoin/bitcoin/issues/29328#issuecomment-1917542993)
> It is pretty easy to tell when the `nTx` value is fake. You can just call [`IsValid()`](https://github.com/bitcoin/bitcoin/blob/478ac185be49feffd1231366c07e06068683f941/src/chain.h#L306), and if it returns true, the `nTx` value is real, and if it returns false, the value is fake.
I don't think this is true. For example, it is possible that a block was downloaded and the `nTx` value was set, but the block later turned out to be invalid. Thus `IsValid()` will return false, but the `nTx` value
...
(https://github.com/bitcoin/bitcoin/issues/29328#issuecomment-1917542993)
> It is pretty easy to tell when the `nTx` value is fake. You can just call [`IsValid()`](https://github.com/bitcoin/bitcoin/blob/478ac185be49feffd1231366c07e06068683f941/src/chain.h#L306), and if it returns true, the `nTx` value is real, and if it returns false, the value is fake.
I don't think this is true. For example, it is possible that a block was downloaded and the `nTx` value was set, but the block later turned out to be invalid. Thus `IsValid()` will return false, but the `nTx` value
...
💬 ishaanam commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1471690474)
Good catch, I think that is possible. Would just using wtxids everywhere here fix this?
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1471690474)
Good catch, I think that is possible. Would just using wtxids everywhere here fix this?
💬 mzumsande commented on issue "assumeutxo: nTx and nChainTx violations in CheckBlockIndex":
(https://github.com/bitcoin/bitcoin/issues/29261#issuecomment-1917606474)
> // If block is assumed valid, nChainTx could be faked, but should at least be increasing between blocks.
I'm not completely convinced this was always true - I might well be wrong (didn't test it), but imagine the following scenario during the background sync:
In the beginning, all relevant blocks are assumed valid and have fake values for `nTx` and `nChainTx`.
Then we receive an out-of order block of height `h` (that doesn't connect yet) -> we set `nTx` and `nChainTx`.
-> Everything is
...
(https://github.com/bitcoin/bitcoin/issues/29261#issuecomment-1917606474)
> // If block is assumed valid, nChainTx could be faked, but should at least be increasing between blocks.
I'm not completely convinced this was always true - I might well be wrong (didn't test it), but imagine the following scenario during the background sync:
In the beginning, all relevant blocks are assumed valid and have fake values for `nTx` and `nChainTx`.
Then we receive an out-of order block of height `h` (that doesn't connect yet) -> we set `nTx` and `nChainTx`.
-> Everything is
...
👍 stickies-v approved a pull request: "Nuke adjusted time from validation (attempt 2)"
(https://github.com/bitcoin/bitcoin/pull/28956#pullrequestreview-1852031524)
ACK ff9039f6ea876bab2c40a06a93e0dd087f445fa2
I would prefer the commit message to contain a rationale for the change, though. It's currently empty.
---
I now believe that the network-adjusted time implementation in master does not offer any security guarantees against an attacker trying to get a node out of consensus. The existing NTP vulnerabilities (which seem to be very real, even if currently apparently not exploited too often) can just as well be used by an attacker when network-ad
...
(https://github.com/bitcoin/bitcoin/pull/28956#pullrequestreview-1852031524)
ACK ff9039f6ea876bab2c40a06a93e0dd087f445fa2
I would prefer the commit message to contain a rationale for the change, though. It's currently empty.
---
I now believe that the network-adjusted time implementation in master does not offer any security guarantees against an attacker trying to get a node out of consensus. The existing NTP vulnerabilities (which seem to be very real, even if currently apparently not exploited too often) can just as well be used by an attacker when network-ad
...
💬 sdaftuar commented on pull request "refactor: Remove nChainTx from consensus":
(https://github.com/bitcoin/bitcoin/pull/29349#discussion_r1471736433)
I'm trying to understand if it's safe to modify these values -- I believe we write the nStatus field to disk and load it on restart, so wouldn't changing these values require a block-index-upgrade mechanism to be created, in order to avoid problems on restart?
(https://github.com/bitcoin/bitcoin/pull/29349#discussion_r1471736433)
I'm trying to understand if it's safe to modify these values -- I believe we write the nStatus field to disk and load it on restart, so wouldn't changing these values require a block-index-upgrade mechanism to be created, in order to avoid problems on restart?