💬 ryanofsky commented on pull request "assumeutxo: Get rid of faked nTx and nChainTx values":
(https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1478766159)
re: https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1477996772
Thanks, removed this
(https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1478766159)
re: https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1477996772
Thanks, removed this
💬 ryanofsky commented on pull request "assumeutxo: Get rid of faked nTx and nChainTx values":
(https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1478757569)
re: https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1477998490
Added a comment. It is necessary to set nSequenceId to avoid an assert failure in CheckBlockIndex. I think it's also a reasonable thing to do to make the block look like it was never received.
(https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1478757569)
re: https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1477998490
Added a comment. It is necessary to set nSequenceId to avoid an assert failure in CheckBlockIndex. I think it's also a reasonable thing to do to make the block look like it was never received.
💬 ryanofsky commented on pull request "assumeutxo: Get rid of faked nTx and nChainTx values":
(https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1478776687)
re: https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1478006900
Thanks, reverted this
(https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1478776687)
re: https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1478006900
Thanks, reverted this
💬 ryanofsky commented on pull request "assumeutxo: Get rid of faked nTx and nChainTx values":
(https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1478947294)
re: https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1478022794
Restored this check
(https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1478947294)
re: https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1478022794
Restored this check
💬 ryanofsky commented on pull request "assumeutxo: Get rid of faked nTx and nChainTx values":
(https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1478766291)
re: https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1477998941
> Same (sequence id)
Thanks, removed this
(https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1478766291)
re: https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1477998941
> Same (sequence id)
Thanks, removed this
⚠️ ThunderCat1000 opened an issue: "Monkey Test Feature (MTF)"
(https://github.com/bitcoin/bitcoin/issues/29389)
### Please describe the feature you'd like to see added.
Consider the following scenario:
Say Chad has a Bitcoin wallet and wants to send a large amount of Bitcoins to another wallet. Chad decides to send a smaller test amount first to make sure everything is OK. Chad proceeds to sign and send the transaction, however, the software has two bugs that transmit his entire balance to a wallet that is not under Chads control. Wouldn't it be nice to be able to perform a formal test (what i call a
...
(https://github.com/bitcoin/bitcoin/issues/29389)
### Please describe the feature you'd like to see added.
Consider the following scenario:
Say Chad has a Bitcoin wallet and wants to send a large amount of Bitcoins to another wallet. Chad decides to send a smaller test amount first to make sure everything is OK. Chad proceeds to sign and send the transaction, however, the software has two bugs that transmit his entire balance to a wallet that is not under Chads control. Wouldn't it be nice to be able to perform a formal test (what i call a
...
👍 jarolrod approved a pull request: "debugwindow: update session ID tooltip"
(https://github.com/bitcoin-core/gui/pull/788#pullrequestreview-1863989093)
ACK 3bf00e13609eefa6ddb11353519bb1aec2342513
(https://github.com/bitcoin-core/gui/pull/788#pullrequestreview-1863989093)
ACK 3bf00e13609eefa6ddb11353519bb1aec2342513
💬 achow101 commented on issue "Monkey Test Feature (MTF)":
(https://github.com/bitcoin/bitcoin/issues/29389#issuecomment-1928516796)
I don't see how this is at all useful. If you are concerned about the existence of such a bug, then you can make the test transactions yourself. However I don't see how such test transactions would safeguard the sender anyways - if there exists a bug that can send all funds in the wallet to someone else, then making a test transaction could hit that bug and send all funds regardless.
A better method is to split up the steps of unsigned transaction creation, transaction signing, and transactio
...
(https://github.com/bitcoin/bitcoin/issues/29389#issuecomment-1928516796)
I don't see how this is at all useful. If you are concerned about the existence of such a bug, then you can make the test transactions yourself. However I don't see how such test transactions would safeguard the sender anyways - if there exists a bug that can send all funds in the wallet to someone else, then making a test transaction could hit that bug and send all funds regardless.
A better method is to split up the steps of unsigned transaction creation, transaction signing, and transactio
...
📝 theStack opened a pull request: "test: speedup bip324_crypto.py unit test"
(https://github.com/bitcoin/bitcoin/pull/29390)
Executing the unit tests for the bip324_crypto.py module currently takes quite long (>60 seconds on my older notebook). Most time here is spent in empty plaintext/ciphertext encryption/decryption loops in `test_fschacha20poly1305aead`:
https://github.com/bitcoin/bitcoin/blob/9eeee7caa3f95ee17a645e12d330261f8e3c2dbf/test/functional/test_framework/crypto/bip324_cipher.py#L193-L194
https://github.com/bitcoin/bitcoin/blob/9eeee7caa3f95ee17a645e12d330261f8e3c2dbf/test/functional/test_framework/cr
...
(https://github.com/bitcoin/bitcoin/pull/29390)
Executing the unit tests for the bip324_crypto.py module currently takes quite long (>60 seconds on my older notebook). Most time here is spent in empty plaintext/ciphertext encryption/decryption loops in `test_fschacha20poly1305aead`:
https://github.com/bitcoin/bitcoin/blob/9eeee7caa3f95ee17a645e12d330261f8e3c2dbf/test/functional/test_framework/crypto/bip324_cipher.py#L193-L194
https://github.com/bitcoin/bitcoin/blob/9eeee7caa3f95ee17a645e12d330261f8e3c2dbf/test/functional/test_framework/cr
...
💬 ThunderCat1000 commented on issue "Monkey Test Feature (MTF)":
(https://github.com/bitcoin/bitcoin/issues/29389#issuecomment-1928583380)
The idea was that there can exist 2 bugs that work together to deplete your wallet when you are merely sending a dust transaction using a normal wallet. A standard feature for wallets to test one part of the transaction in isolation (the send to address function) without being impacted by other code that has a dynamic amount for the transaction, would give peace of mind without the complexity of the process you are suggesting. Hence the name "Monkey Test"
(https://github.com/bitcoin/bitcoin/issues/29389#issuecomment-1928583380)
The idea was that there can exist 2 bugs that work together to deplete your wallet when you are merely sending a dust transaction using a normal wallet. A standard feature for wallets to test one part of the transaction in isolation (the send to address function) without being impacted by other code that has a dynamic amount for the transaction, would give peace of mind without the complexity of the process you are suggesting. Hence the name "Monkey Test"
💬 mzumsande commented on pull request "assumeutxo: Get rid of faked nTx and nChainTx values":
(https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1479169504)
Would `GuessVerificationProgress` lose accuracy with the removal or was the comment wrong?
(https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1479169504)
Would `GuessVerificationProgress` lose accuracy with the removal or was the comment wrong?
💬 jo-elimu commented on pull request "test: speedup bip324_cipher.py unit test":
(https://github.com/bitcoin/bitcoin/pull/29390#issuecomment-1928764743)
Is this pull request associated with one of the issues at https://github.com/bitcoin/bitcoin/issues?
(https://github.com/bitcoin/bitcoin/pull/29390#issuecomment-1928764743)
Is this pull request associated with one of the issues at https://github.com/bitcoin/bitcoin/issues?
💬 jo-elimu commented on pull request "test: fix RPC coverage check":
(https://github.com/bitcoin/bitcoin/pull/29387#discussion_r1479210925)
@BrandonOdiwuor What does the `-disablewallet` mean?
(https://github.com/bitcoin/bitcoin/pull/29387#discussion_r1479210925)
@BrandonOdiwuor What does the `-disablewallet` mean?
💬 maflcko commented on pull request "fuzz: remove unused `args` and `context` from `FuzzedWallet`":
(https://github.com/bitcoin/bitcoin/pull/29388#issuecomment-1928938045)
lgtm ACK b14298c5bca395e5ed6a27fe1758c0d1f4b824ac
(https://github.com/bitcoin/bitcoin/pull/29388#issuecomment-1928938045)
lgtm ACK b14298c5bca395e5ed6a27fe1758c0d1f4b824ac
💬 delta1 commented on pull request "test: speedup bip324_cipher.py unit test":
(https://github.com/bitcoin/bitcoin/pull/29390#issuecomment-1928950663)
Concept ACK a8c3454
To run the individual test I had to run it from the `test/functional` directory this way: `python3 -m unittest ./crypto/bip324_cipher.py`
Saves about 10 seconds in test_runner on my machine
Before:
```
Running Unit Tests for Test Framework Modules
.....................
----------------------------------------------------------------------
Ran 21 tests in 15.701s
```
After:
```
Running Unit Tests for Test Framework Modules
.....................
--------
...
(https://github.com/bitcoin/bitcoin/pull/29390#issuecomment-1928950663)
Concept ACK a8c3454
To run the individual test I had to run it from the `test/functional` directory this way: `python3 -m unittest ./crypto/bip324_cipher.py`
Saves about 10 seconds in test_runner on my machine
Before:
```
Running Unit Tests for Test Framework Modules
.....................
----------------------------------------------------------------------
Ran 21 tests in 15.701s
```
After:
```
Running Unit Tests for Test Framework Modules
.....................
--------
...
💬 maflcko commented on pull request "test: fix RPC coverage check":
(https://github.com/bitcoin/bitcoin/pull/29387#discussion_r1479350456)
You can remove this? Also, it would be good to mention the commit that this reverts. (It may be one of mine)
(https://github.com/bitcoin/bitcoin/pull/29387#discussion_r1479350456)
You can remove this? Also, it would be good to mention the commit that this reverts. (It may be one of mine)
💬 maflcko commented on pull request "util: check for errors after close and read in AutoFile":
(https://github.com/bitcoin/bitcoin/pull/29307#issuecomment-1928966222)
> How/when would the file be closed?
An `AutoFile` can be closed by calling the `fclose()` method in the code.
(https://github.com/bitcoin/bitcoin/pull/29307#issuecomment-1928966222)
> How/when would the file be closed?
An `AutoFile` can be closed by calling the `fclose()` method in the code.
💬 maflcko commented on pull request "assumeutxo: Get rid of faked nTx and nChainTx values":
(https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1479427801)
I think the comment was wrong, or do you have a code path where nChainTx is read when it was wrong (previously) or is unset (now)?
Generally, `GuessVerificationProgress` is only called to estimate the IBD progress at the tip, in which case nChainTx is set to the correct value. It is also used to guess rescan progress, which is not possible for blocks outside the active chain or blocks that have data missing, so nChainTx should be correct there as well.
unrelated to this pull: Maybe an `Ass
...
(https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1479427801)
I think the comment was wrong, or do you have a code path where nChainTx is read when it was wrong (previously) or is unset (now)?
Generally, `GuessVerificationProgress` is only called to estimate the IBD progress at the tip, in which case nChainTx is set to the correct value. It is also used to guess rescan progress, which is not possible for blocks outside the active chain or blocks that have data missing, so nChainTx should be correct there as well.
unrelated to this pull: Maybe an `Ass
...
💬 maflcko commented on pull request "assumeutxo: Get rid of faked nTx and nChainTx values":
(https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1479428612)
unrelated to this pull: May be better to throw here, instead of returning wrong data, but this can be done in a follow-up.
(https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1479428612)
unrelated to this pull: May be better to throw here, instead of returning wrong data, but this can be done in a follow-up.
💬 maflcko commented on pull request "assumeutxo: Get rid of faked nTx and nChainTx values":
(https://github.com/bitcoin/bitcoin/pull/29370#issuecomment-1929064411)
Needs rebase
(https://github.com/bitcoin/bitcoin/pull/29370#issuecomment-1929064411)
Needs rebase