🤔 ishaanam reviewed a pull request: "wallet: Deniability API (Unilateral Transaction Meta-Privacy)"
(https://github.com/bitcoin/bitcoin/pull/27792#pullrequestreview-1519449241)
Hey @denavila, welcome to Bitcoin Core and thanks for working on this! While I think that this idea could be useful to some extent if implemented correctly, I have a few concerns about the current implementation:
- Currently there is no precaution against a user spending two outputs of a deniabilization transaction together. IMO I think that this is quite problematic and could lead to these deniabilization transactions being rendered ineffective. Not only that, but then it also becomes obvious
...
(https://github.com/bitcoin/bitcoin/pull/27792#pullrequestreview-1519449241)
Hey @denavila, welcome to Bitcoin Core and thanks for working on this! While I think that this idea could be useful to some extent if implemented correctly, I have a few concerns about the current implementation:
- Currently there is no precaution against a user spending two outputs of a deniabilization transaction together. IMO I think that this is quite problematic and could lead to these deniabilization transactions being rendered ineffective. Not only that, but then it also becomes obvious
...
💬 theuni commented on pull request "kernel: Remove StartShutdown calls from validation code":
(https://github.com/bitcoin/bitcoin/pull/28048#issuecomment-1625907483)
Very nice. Concept ACK and quick (very shallow) codereview ACK.
I think the `-stopatheight` change is reasonable. If it turns out anyone/anything was relying on the way this worked before, we could always add functionality for that use-case (maybe `-stopatblock`?)
However, the help string should be updated from:
> Stop running after reaching the given height in the main chain
It sounds like it's not completely accurate now, and less so after this change :)
(https://github.com/bitcoin/bitcoin/pull/28048#issuecomment-1625907483)
Very nice. Concept ACK and quick (very shallow) codereview ACK.
I think the `-stopatheight` change is reasonable. If it turns out anyone/anything was relying on the way this worked before, we could always add functionality for that use-case (maybe `-stopatblock`?)
However, the help string should be updated from:
> Stop running after reaching the given height in the main chain
It sounds like it's not completely accurate now, and less so after this change :)
💬 achow101 commented on pull request "Add support for RFC8439 variant of ChaCha20":
(https://github.com/bitcoin/bitcoin/pull/27985#issuecomment-1625915605)
Assuming it doesn't break a whole lot of tests, I would prefer to change it always use the new version as it removes the need to check that everything is using the right variant.
(https://github.com/bitcoin/bitcoin/pull/27985#issuecomment-1625915605)
Assuming it doesn't break a whole lot of tests, I would prefer to change it always use the new version as it removes the need to check that everything is using the right variant.
💬 jonatack commented on pull request "test: move remaining rand code from util/setup_common to util/random":
(https://github.com/bitcoin/bitcoin/pull/27425#issuecomment-1625936888)
Thank you for the reviews @TheCharlatan and @MarcoFalke. Updated the name of the new helper and removed the static annotations per https://github.com/bitcoin/bitcoin/pull/27425#discussion_r1250420188, should be trivial to re-ACK.
(https://github.com/bitcoin/bitcoin/pull/27425#issuecomment-1625936888)
Thank you for the reviews @TheCharlatan and @MarcoFalke. Updated the name of the new helper and removed the static annotations per https://github.com/bitcoin/bitcoin/pull/27425#discussion_r1250420188, should be trivial to re-ACK.
💬 theuni commented on pull request "kernel: Remove StartShutdown calls from validation code":
(https://github.com/bitcoin/bitcoin/pull/28048#discussion_r1256350258)
Is it possible for this to be called twice?
```c++
if (m_stop_at_height) {
if (index.nHeight > m_stop_at_height) {
// assert(false) or return kernel::Interrupted{} ?
} else if (index.nHeight == m_stop_at_height) {
..
}
}
```
Not that I imagine calling `StartShutdown()` twice would be particularly harmful anyway.
(https://github.com/bitcoin/bitcoin/pull/28048#discussion_r1256350258)
Is it possible for this to be called twice?
```c++
if (m_stop_at_height) {
if (index.nHeight > m_stop_at_height) {
// assert(false) or return kernel::Interrupted{} ?
} else if (index.nHeight == m_stop_at_height) {
..
}
}
```
Not that I imagine calling `StartShutdown()` twice would be particularly harmful anyway.
⚠️ dsldsl opened an issue: "Malware reported in scan of bitcoin-qt versions 22 and 24.0.1 apple darwin"
(https://github.com/bitcoin/bitcoin/issues/28049)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
<img width="1174" alt="image" src="https://github.com/bitcoin/bitcoin/assets/50015/625e57b0-6a86-486d-b18a-58c93ce60f26">
My virus scanner reported the malware Koiot found in the dmg file and in the installed app of bitcoin core. Is this a known false positive or an actual potential issue with a bad binary I downloaded? thank you.
### Expected behaviour
Not have a virus
### Steps t
...
(https://github.com/bitcoin/bitcoin/issues/28049)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
<img width="1174" alt="image" src="https://github.com/bitcoin/bitcoin/assets/50015/625e57b0-6a86-486d-b18a-58c93ce60f26">
My virus scanner reported the malware Koiot found in the dmg file and in the installed app of bitcoin core. Is this a known false positive or an actual potential issue with a bad binary I downloaded? thank you.
### Expected behaviour
Not have a virus
### Steps t
...
🤔 ryanofsky reviewed a pull request: "blockstorage: Return on fatal flush errors"
(https://github.com/bitcoin/bitcoin/pull/27866#pullrequestreview-1519541703)
Code review d1c92b57a72b62ffa202f1d3d357b59befdc9c12. I think the extra error reporting here is great, and some of the changes in behavior seem good too. But I think there is too much happening in one commit. There are 3-4 separate changes in behavior that could be implemented in separate commits, and I think would be more clearly understood and evaluated that way.
(https://github.com/bitcoin/bitcoin/pull/27866#pullrequestreview-1519541703)
Code review d1c92b57a72b62ffa202f1d3d357b59befdc9c12. I think the extra error reporting here is great, and some of the changes in behavior seem good too. But I think there is too much happening in one commit. There are 3-4 separate changes in behavior that could be implemented in separate commits, and I think would be more clearly understood and evaluated that way.
💬 ryanofsky commented on pull request "blockstorage: Return on fatal flush errors":
(https://github.com/bitcoin/bitcoin/pull/27866#discussion_r1256329428)
In commit "blockstorage: Return on fatal flush errors" (d1c92b57a72b62ffa202f1d3d357b59befdc9c12)
This skips trying to flush the undo file if flushing the block file fails. It think it would be safer and more conservative to keep previous behavior of flushing both files in sequence.
You can still return false if either flush call fails, but I don't see a benefit in changing behavior here and potentially making code less robust, when it's not necessary just to add a return code.
I think
...
(https://github.com/bitcoin/bitcoin/pull/27866#discussion_r1256329428)
In commit "blockstorage: Return on fatal flush errors" (d1c92b57a72b62ffa202f1d3d357b59befdc9c12)
This skips trying to flush the undo file if flushing the block file fails. It think it would be safer and more conservative to keep previous behavior of flushing both files in sequence.
You can still return false if either flush call fails, but I don't see a benefit in changing behavior here and potentially making code less robust, when it's not necessary just to add a return code.
I think
...
💬 ryanofsky commented on pull request "blockstorage: Return on fatal flush errors":
(https://github.com/bitcoin/bitcoin/pull/27866#discussion_r1256284494)
In commit "blockstorage: Return on fatal flush errors" (d1c92b57a72b62ffa202f1d3d357b59befdc9c12)
Let me know if I understand this correctly: The new `return false` avoids writing block data to a new block file if the _previous_ block file is full and cannot be synced and trimmed. This seems to be safe because this `FindBlockPos` function only has one caller, `SaveBlockToDisk`, which only has two callers, `Chainstate::AcceptBlock` and `Chainstate::LoadGenesisBlock` which both seem to handle t
...
(https://github.com/bitcoin/bitcoin/pull/27866#discussion_r1256284494)
In commit "blockstorage: Return on fatal flush errors" (d1c92b57a72b62ffa202f1d3d357b59befdc9c12)
Let me know if I understand this correctly: The new `return false` avoids writing block data to a new block file if the _previous_ block file is full and cannot be synced and trimmed. This seems to be safe because this `FindBlockPos` function only has one caller, `SaveBlockToDisk`, which only has two callers, `Chainstate::AcceptBlock` and `Chainstate::LoadGenesisBlock` which both seem to handle t
...
💬 ryanofsky commented on pull request "blockstorage: Return on fatal flush errors":
(https://github.com/bitcoin/bitcoin/pull/27866#discussion_r1256350394)
In commit "blockstorage: Return on fatal flush errors" (d1c92b57a72b62ffa202f1d3d357b59befdc9c12)
This change seems fragile to me, and it's unclear what the benefits are. I think it would be great to add a log print here, but if this this code is going to be changed to return false, I think that should happen in a separate followup commit, not combined with other changes here, and have a clear explanation and justification.
IIUC, this change introduces an inconsistency. For all block data,
...
(https://github.com/bitcoin/bitcoin/pull/27866#discussion_r1256350394)
In commit "blockstorage: Return on fatal flush errors" (d1c92b57a72b62ffa202f1d3d357b59befdc9c12)
This change seems fragile to me, and it's unclear what the benefits are. I think it would be great to add a log print here, but if this this code is going to be changed to return false, I think that should happen in a separate followup commit, not combined with other changes here, and have a clear explanation and justification.
IIUC, this change introduces an inconsistency. For all block data,
...
💬 ryanofsky commented on pull request "blockstorage: Return on fatal flush errors":
(https://github.com/bitcoin/bitcoin/pull/27866#discussion_r1256378064)
In commit "blockstorage: Return on fatal flush errors" (d1c92b57a72b62ffa202f1d3d357b59befdc9c12)
This line appears to implement the major changing in behavior which is avoiding the "write from
`WriteBlockIndexDB` that may refer to a block that is not fully flushed to disk yet."
I wonder what this failure looks like in practice, though. Could we write a test for it? I think it would at least make sense to have a log print that explains the problem that's happening.
There are also a *l
...
(https://github.com/bitcoin/bitcoin/pull/27866#discussion_r1256378064)
In commit "blockstorage: Return on fatal flush errors" (d1c92b57a72b62ffa202f1d3d357b59befdc9c12)
This line appears to implement the major changing in behavior which is avoiding the "write from
`WriteBlockIndexDB` that may refer to a block that is not fully flushed to disk yet."
I wonder what this failure looks like in practice, though. Could we write a test for it? I think it would at least make sense to have a log print that explains the problem that's happening.
There are also a *l
...
📝 furszy opened a pull request: "test: make assumeUTXO test capture the expected fatal error"
(https://github.com/bitcoin/bitcoin/pull/28050)
The test is exercising the error, so it can capture it before the
test framework displays it on the console as an unforeseen
fatal error.
It is odd to observe a fatal error after executing the complete
test suite.
(https://github.com/bitcoin/bitcoin/pull/28050)
The test is exercising the error, so it can capture it before the
test framework displays it on the console as an unforeseen
fatal error.
It is odd to observe a fatal error after executing the complete
test suite.
💬 TheCharlatan commented on pull request "kernel: Remove StartShutdown calls from validation code":
(https://github.com/bitcoin/bitcoin/pull/28048#issuecomment-1626127733)
Strong Concept ACK :)
I'm glad you changed your mind on returning things from the notifications, putting the infrastructure in place to allow the code to now bubble a `-stopatblock` interrupt is a clear advantage to me.
I was about to open a similar PR, albeit with a different approach for handling `-stopafterblockimport` ([patch](https://github.com/TheCharlatan/bitcoin/commit/1107a234838d7a3999dbf236c8a7e4e00d48f289)). Instead of creating a new notification for it, I thought slightly refa
...
(https://github.com/bitcoin/bitcoin/pull/28048#issuecomment-1626127733)
Strong Concept ACK :)
I'm glad you changed your mind on returning things from the notifications, putting the infrastructure in place to allow the code to now bubble a `-stopatblock` interrupt is a clear advantage to me.
I was about to open a similar PR, albeit with a different approach for handling `-stopafterblockimport` ([patch](https://github.com/TheCharlatan/bitcoin/commit/1107a234838d7a3999dbf236c8a7e4e00d48f289)). Instead of creating a new notification for it, I thought slightly refa
...
💬 jonatack commented on pull request "test: cover addrv2 anchors by adding TorV3 to CAddress in messages.py":
(https://github.com/bitcoin/bitcoin/pull/27452#discussion_r1256352616)
f6bd653
<details><summary>suggested diff</summary><p>
```diff
@@ -76,11 +76,11 @@ TEST_FRAMEWORK_MODULES = [
"blocktools",
"ellswift",
"key",
+ "messages",
"muhash",
"ripemd160",
"script",
"segwit_addr",
- "messages"
]
```
</p></details>
(Also, project convention is to leave a comma after the last element, to minimize the diff in future changes)
(https://github.com/bitcoin/bitcoin/pull/27452#discussion_r1256352616)
f6bd653
<details><summary>suggested diff</summary><p>
```diff
@@ -76,11 +76,11 @@ TEST_FRAMEWORK_MODULES = [
"blocktools",
"ellswift",
"key",
+ "messages",
"muhash",
"ripemd160",
"script",
"segwit_addr",
- "messages"
]
```
</p></details>
(Also, project convention is to leave a comma after the last element, to minimize the diff in future changes)
🤔 jonatack reviewed a pull request: "test: cover addrv2 anchors by adding TorV3 to CAddress in messages.py"
(https://github.com/bitcoin/bitcoin/pull/27452#pullrequestreview-1519649211)
Tested Approach ACK
(https://github.com/bitcoin/bitcoin/pull/27452#pullrequestreview-1519649211)
Tested Approach ACK
💬 jonatack commented on pull request "test: cover addrv2 anchors by adding TorV3 to CAddress in messages.py":
(https://github.com/bitcoin/bitcoin/pull/27452#discussion_r1256494108)
805692f
<details><summary>Various suggestions</summary><p>
```diff
diff --git a/test/functional/feature_anchors.py b/test/functional/feature_anchors.py
index 6c97eed9d86..87c6082b3e0 100755
--- a/test/functional/feature_anchors.py
+++ b/test/functional/feature_anchors.py
@@ -14,6 +14,7 @@ from test_framework.util import check_node_connections, assert_equal, p2p_port
INBOUND_CONNECTIONS = 5
BLOCK_RELAY_CONNECTIONS = 2
+ONION_ADDR = "pg6mmjiyjmcrsslvykfwnntlaru7p5svn6y2ymmju6nub
...
(https://github.com/bitcoin/bitcoin/pull/27452#discussion_r1256494108)
805692f
<details><summary>Various suggestions</summary><p>
```diff
diff --git a/test/functional/feature_anchors.py b/test/functional/feature_anchors.py
index 6c97eed9d86..87c6082b3e0 100755
--- a/test/functional/feature_anchors.py
+++ b/test/functional/feature_anchors.py
@@ -14,6 +14,7 @@ from test_framework.util import check_node_connections, assert_equal, p2p_port
INBOUND_CONNECTIONS = 5
BLOCK_RELAY_CONNECTIONS = 2
+ONION_ADDR = "pg6mmjiyjmcrsslvykfwnntlaru7p5svn6y2ymmju6nub
...
💬 jonatack commented on pull request "test: cover addrv2 anchors by adding TorV3 to CAddress in messages.py":
(https://github.com/bitcoin/bitcoin/pull/27452#discussion_r1256380724)
https://github.com/bitcoin/bitcoin/commit/f6bd653b161749d4a87de9ef4a91bc32d4b3def3 nit, can be simplified
<details><summary>diff</summary><p>
```diff
def check_addrv2(ip, net):
addr = CAddress()
- addr.net = net
- addr.ip = ip
+ addr.net, addr.ip = net, ip
ser = addr.serialize_v2()
actual = CAddress()
actual.deserialize_v2(BytesIO(ser))
- self.assertEqual(actual.ip, ip)
-
...
(https://github.com/bitcoin/bitcoin/pull/27452#discussion_r1256380724)
https://github.com/bitcoin/bitcoin/commit/f6bd653b161749d4a87de9ef4a91bc32d4b3def3 nit, can be simplified
<details><summary>diff</summary><p>
```diff
def check_addrv2(ip, net):
addr = CAddress()
- addr.net = net
- addr.ip = ip
+ addr.net, addr.ip = net, ip
ser = addr.serialize_v2()
actual = CAddress()
actual.deserialize_v2(BytesIO(ser))
- self.assertEqual(actual.ip, ip)
-
...
💬 jonatack commented on pull request "test: cover addrv2 anchors by adding TorV3 to CAddress in messages.py":
(https://github.com/bitcoin/bitcoin/pull/27452#discussion_r1256363333)
https://github.com/bitcoin/bitcoin/commit/f6bd653b161749d4a87de9ef4a91bc32d4b3def3 naming, this would be more readable in case of a test failure (also, remove 2 extra LF at EOF)
<details><summary>diff</summary><p>
```diff
class TestFrameworkScript(unittest.TestCase):
- def test_addrv2encodedecode(self):
+ def test_addrv2_encode_decode(self):
def check_addrv2(ip, net):
@@ -1905,5 +1905,3 @@ class TestFrameworkScript(unittest.TestCase):
check_addrv2("fc32:17e
...
(https://github.com/bitcoin/bitcoin/pull/27452#discussion_r1256363333)
https://github.com/bitcoin/bitcoin/commit/f6bd653b161749d4a87de9ef4a91bc32d4b3def3 naming, this would be more readable in case of a test failure (also, remove 2 extra LF at EOF)
<details><summary>diff</summary><p>
```diff
class TestFrameworkScript(unittest.TestCase):
- def test_addrv2encodedecode(self):
+ def test_addrv2_encode_decode(self):
def check_addrv2(ip, net):
@@ -1905,5 +1905,3 @@ class TestFrameworkScript(unittest.TestCase):
check_addrv2("fc32:17e
...
💬 jonatack commented on pull request "test: cover addrv2 anchors by adding TorV3 to CAddress in messages.py":
(https://github.com/bitcoin/bitcoin/pull/27452#discussion_r1256496328)
I think it would be helpful to explain this in the test.
(https://github.com/bitcoin/bitcoin/pull/27452#discussion_r1256496328)
I think it would be helpful to explain this in the test.
💬 ryanofsky commented on pull request "kernel: Remove StartShutdown calls from validation code":
(https://github.com/bitcoin/bitcoin/pull/28048#discussion_r1256406548)
re: https://github.com/bitcoin/bitcoin/pull/28048#discussion_r1256350258
I don't think StartShutdown() can be called twice, but it should be ok if that did happen, like you say.
Checking `index.nHeight > m_stop_at_height` wouldn't be a good way to detect two shutdown calls, though, because that condition can also happen if they the node is restarted with a lower `-stopatheight` value (because the implementation only stops when new blocks are added, and doesn't make any attempt to rewind)
(https://github.com/bitcoin/bitcoin/pull/28048#discussion_r1256406548)
re: https://github.com/bitcoin/bitcoin/pull/28048#discussion_r1256350258
I don't think StartShutdown() can be called twice, but it should be ok if that did happen, like you say.
Checking `index.nHeight > m_stop_at_height` wouldn't be a good way to detect two shutdown calls, though, because that condition can also happen if they the node is restarted with a lower `-stopatheight` value (because the implementation only stops when new blocks are added, and doesn't make any attempt to rewind)
🤔 ryanofsky reviewed a pull request: "kernel: Remove StartShutdown calls from validation code"
(https://github.com/bitcoin/bitcoin/pull/28048#pullrequestreview-1519716758)
Updated c2bcc339d7b8def58289d3ff01b2a905dc291ed6 -> 09938a41d904c05b4676b064da9baa85e53e3e6f ([`pr/stopafter.2`](https://github.com/ryanofsky/bitcoin/commits/pr/stopafter.2) -> [`pr/stopafter.3`](https://github.com/ryanofsky/bitcoin/commits/pr/stopafter.3), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/stopafter.2..pr/stopafter.3)) updating -stopatheight documentation to be a little more precise.
I was also going to add a new commit 03dfa85e057192e38d706b8c40a21e70c5aeec91 droppin
...
(https://github.com/bitcoin/bitcoin/pull/28048#pullrequestreview-1519716758)
Updated c2bcc339d7b8def58289d3ff01b2a905dc291ed6 -> 09938a41d904c05b4676b064da9baa85e53e3e6f ([`pr/stopafter.2`](https://github.com/ryanofsky/bitcoin/commits/pr/stopafter.2) -> [`pr/stopafter.3`](https://github.com/ryanofsky/bitcoin/commits/pr/stopafter.3), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/stopafter.2..pr/stopafter.3)) updating -stopatheight documentation to be a little more precise.
I was also going to add a new commit 03dfa85e057192e38d706b8c40a21e70c5aeec91 droppin
...