💬 real-or-random commented on pull request "Add ASM optimizations for MuHash3072":
(https://github.com/bitcoin/bitcoin/pull/19181#issuecomment-1801703220)
There's a lot of activity recently in GCC towards generating adc instructions on x86(_64): https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79173 This thread also has some hints at possible other implementations such as [GCC builtins](https://gcc.gnu.org/onlinedocs/gcc/Integer-Overflow-Builtins.html), which may be simpler to review.
I don't know. Checking the asm annotations is not trivial and not many people are familiar with inline asm. But on the other hand, I'm not saying that this PR is a h
...
(https://github.com/bitcoin/bitcoin/pull/19181#issuecomment-1801703220)
There's a lot of activity recently in GCC towards generating adc instructions on x86(_64): https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79173 This thread also has some hints at possible other implementations such as [GCC builtins](https://gcc.gnu.org/onlinedocs/gcc/Integer-Overflow-Builtins.html), which may be simpler to review.
I don't know. Checking the asm annotations is not trivial and not many people are familiar with inline asm. But on the other hand, I'm not saying that this PR is a h
...
🚀 glozow merged a pull request: "net: improves addnode / m_added_nodes logic"
(https://github.com/bitcoin/bitcoin/pull/28155)
(https://github.com/bitcoin/bitcoin/pull/28155)
💬 maflcko commented on pull request "fuzz: Avoid timeout and bloat in fuzz targets":
(https://github.com/bitcoin/bitcoin/pull/28815#issuecomment-1801829102)
Not sure, there are also some loops that do not pick out of several callables. Happy to push any refactoring, if someone uploads a diff, patch, or commit.
(https://github.com/bitcoin/bitcoin/pull/28815#issuecomment-1801829102)
Not sure, there are also some loops that do not pick out of several callables. Happy to push any refactoring, if someone uploads a diff, patch, or commit.
👍 dergoegge approved a pull request: "fuzz: Avoid timeout and bloat in fuzz targets"
(https://github.com/bitcoin/bitcoin/pull/28815#pullrequestreview-1720320820)
utACK fabb5046a7365af3079e6e45606d63576bc6ad12
Didn't test but the rational makes a lot of sense to me. This should be better even if it doesn't solve the timeouts.
(https://github.com/bitcoin/bitcoin/pull/28815#pullrequestreview-1720320820)
utACK fabb5046a7365af3079e6e45606d63576bc6ad12
Didn't test but the rational makes a lot of sense to me. This should be better even if it doesn't solve the timeouts.
👍 ismaelsadeeq approved a pull request: "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access"
(https://github.com/bitcoin/bitcoin/pull/28391#pullrequestreview-1720237245)
re ACK 995aa6b9cb5d1ea685a5d2ac6767d21b373e4c84
(https://github.com/bitcoin/bitcoin/pull/28391#pullrequestreview-1720237245)
re ACK 995aa6b9cb5d1ea685a5d2ac6767d21b373e4c84
💬 ismaelsadeeq commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1386552287)
Assert?
```suggestion
AssertLockHeld(cs);
indexed_transaction_set::const_iterator i = mapTx.find(hash);
```
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1386552287)
Assert?
```suggestion
AssertLockHeld(cs);
indexed_transaction_set::const_iterator i = mapTx.find(hash);
```
💬 ismaelsadeeq commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1386609897)
nit
https://github.com/bitcoin/bitcoin/blob/6903d8aa37e01d16aea0b53c3729f8f92c7d2bf0/src/test/miniminer_tests.cpp#L97
Can also be
```cpp
BOOST_CHECK(negative_modified_fees < feerate_zero.GetFee(pool.GetEntry(tx_mod_negative->GetHash())->GetTxSize()));
```
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1386609897)
nit
https://github.com/bitcoin/bitcoin/blob/6903d8aa37e01d16aea0b53c3729f8f92c7d2bf0/src/test/miniminer_tests.cpp#L97
Can also be
```cpp
BOOST_CHECK(negative_modified_fees < feerate_zero.GetFee(pool.GetEntry(tx_mod_negative->GetHash())->GetTxSize()));
```
👍 brunoerg approved a pull request: "fuzz: Avoid timeout and bloat in fuzz targets"
(https://github.com/bitcoin/bitcoin/pull/28815#pullrequestreview-1720390505)
crACK fabb5046a7365af3079e6e45606d63576bc6ad12
Logic makes sense for me. I didn't check all the targets, but it seems we could apply it in `coincontrol` as well.
(https://github.com/bitcoin/bitcoin/pull/28815#pullrequestreview-1720390505)
crACK fabb5046a7365af3079e6e45606d63576bc6ad12
Logic makes sense for me. I didn't check all the targets, but it seems we could apply it in `coincontrol` as well.
💬 maflcko commented on pull request "fuzz: Avoid timeout and bloat in fuzz targets":
(https://github.com/bitcoin/bitcoin/pull/28815#issuecomment-1801931728)
> Logic makes sense for me. I didn't check all the targets, but it seems we could apply it in `coincontrol` as well.
Yes, I was thinking about this, too. In practise it doesn't matter because `COutPoint` happens to be (de)serializes as a fixed-size byte blob, which can only fail if the fuzz provider is out of data. In that case the loop will already abort. However, it could make sense to change this for consistency as well, or to protect against the case where `COutPoint` is "dynamically" des
...
(https://github.com/bitcoin/bitcoin/pull/28815#issuecomment-1801931728)
> Logic makes sense for me. I didn't check all the targets, but it seems we could apply it in `coincontrol` as well.
Yes, I was thinking about this, too. In practise it doesn't matter because `COutPoint` happens to be (de)serializes as a fixed-size byte blob, which can only fail if the fuzz provider is out of data. In that case the loop will already abort. However, it could make sense to change this for consistency as well, or to protect against the case where `COutPoint` is "dynamically" des
...
💬 brunoerg commented on pull request "fuzz: add target for `DescriptorScriptPubKeyMan`":
(https://github.com/bitcoin/bitcoin/pull/28578#issuecomment-1801958804)
Force-pushed covering more functions. Left it running for a long time and didn't see any downside.
(https://github.com/bitcoin/bitcoin/pull/28578#issuecomment-1801958804)
Force-pushed covering more functions. Left it running for a long time and didn't see any downside.
💬 maflcko commented on pull request "Show transactions as not fully confirmed during background validation":
(https://github.com/bitcoin/bitcoin/pull/28616#discussion_r1386694325)
Did that for fun, and it prints the error message:
```
test_framework.authproxy.JSONRPCException: Wallet loading failed. Error loading wallet. Wallet requires blocks to be downloaded, and software does not currently support loading wallets while blocks are being downloaded out of order when using assumeutxo snapshots. Wallet should be able to load successfully after node sync reaches height 299 (-4)
```
Hacky diff:
```diff
diff --git a/test/functional/feature_assumeutxo.py b/test/fun
...
(https://github.com/bitcoin/bitcoin/pull/28616#discussion_r1386694325)
Did that for fun, and it prints the error message:
```
test_framework.authproxy.JSONRPCException: Wallet loading failed. Error loading wallet. Wallet requires blocks to be downloaded, and software does not currently support loading wallets while blocks are being downloaded out of order when using assumeutxo snapshots. Wallet should be able to load successfully after node sync reaches height 299 (-4)
```
Hacky diff:
```diff
diff --git a/test/functional/feature_assumeutxo.py b/test/fun
...
🚀 fanquake merged a pull request: "fuzz: Avoid timeout and bloat in fuzz targets"
(https://github.com/bitcoin/bitcoin/pull/28815)
(https://github.com/bitcoin/bitcoin/pull/28815)
💬 maflcko commented on issue "test: Write assumeutxo tests":
(https://github.com/bitcoin/bitcoin/issues/28648#issuecomment-1801990207)
Another idea would be to start a new wallet test. For now it could only check that it throws an error. See the example hacky diff I wrote, which will need to be moved to a new test file: https://github.com/bitcoin/bitcoin/pull/28616#discussion_r1386694325
(https://github.com/bitcoin/bitcoin/issues/28648#issuecomment-1801990207)
Another idea would be to start a new wallet test. For now it could only check that it throws an error. See the example hacky diff I wrote, which will need to be moved to a new test file: https://github.com/bitcoin/bitcoin/pull/28616#discussion_r1386694325
📝 fanquake opened a pull request: "ci: remove note re M1 usage"
(https://github.com/bitcoin/bitcoin/pull/28823)
M1 is now available in GitHub CI, but we don't currently have a plan to use it, so remove the comment.
(https://github.com/bitcoin/bitcoin/pull/28823)
M1 is now available in GitHub CI, but we don't currently have a plan to use it, so remove the comment.
💬 jsarenik commented on issue "v26.0 Testing":
(https://github.com/bitcoin/bitcoin/issues/28718#issuecomment-1802095699)
Running two 26rc2 nodes, using binaries from bitcoincore.org, one on aarch64, one on amd64. Both are also on Tor and v2transport-enabled, connected via v2 at least to one another node running daily master.
Apart from local bitcoin-cli connections keeping bitcoind from restarting, which is present for some time since around v25 on master, everything works fine. Thanks.
(https://github.com/bitcoin/bitcoin/issues/28718#issuecomment-1802095699)
Running two 26rc2 nodes, using binaries from bitcoincore.org, one on aarch64, one on amd64. Both are also on Tor and v2transport-enabled, connected via v2 at least to one another node running daily master.
Apart from local bitcoin-cli connections keeping bitcoind from restarting, which is present for some time since around v25 on master, everything works fine. Thanks.
💬 stratospher commented on pull request "test: Make existing functional tests compatible with --v2transport":
(https://github.com/bitcoin/bitcoin/pull/28805#issuecomment-1802144035)
Nice! Concept ACK.
> "Before, a global -v2transport provided to the test would be dropped when restarting the node within a test."
@mzumsande, wanted to confirm which scenario you're talking about. i tried it out in [this file](https://github.com/stratospher/bitcoin/blob/ab72033bdf9e3e2155fe645a1c4b71d0d6249a57/test/functional/test_v2transport.py).
1. when test is run with `--v2transport` option and `TestNode` (with no `extra_args` passed in the test) is restarted? `TestNode` restarted
...
(https://github.com/bitcoin/bitcoin/pull/28805#issuecomment-1802144035)
Nice! Concept ACK.
> "Before, a global -v2transport provided to the test would be dropped when restarting the node within a test."
@mzumsande, wanted to confirm which scenario you're talking about. i tried it out in [this file](https://github.com/stratospher/bitcoin/blob/ab72033bdf9e3e2155fe645a1c4b71d0d6249a57/test/functional/test_v2transport.py).
1. when test is run with `--v2transport` option and `TestNode` (with no `extra_args` passed in the test) is restarted? `TestNode` restarted
...
🤔 glozow reviewed a pull request: "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access"
(https://github.com/bitcoin/bitcoin/pull/28391#pullrequestreview-1720647559)
LGTM aedfef28af6e8c5bd8833fb63e2ef4f3264f934c, just a couple introductions of uint256 that should probably be the type-safe ones instead
(https://github.com/bitcoin/bitcoin/pull/28391#pullrequestreview-1720647559)
LGTM aedfef28af6e8c5bd8833fb63e2ef4f3264f934c, just a couple introductions of uint256 that should probably be the type-safe ones instead
💬 glozow commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1386805310)
6903d8aa37e01d16aea0b53c3729f8f92c7d2bf0
Could you use `Txid` instead? or `GenTxid`
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1386805310)
6903d8aa37e01d16aea0b53c3729f8f92c7d2bf0
Could you use `Txid` instead? or `GenTxid`
💬 glozow commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1386815017)
Wow TIL there's a `get`!
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1386815017)
Wow TIL there's a `get`!
💬 glozow commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1386820528)
aedfef28af6e8c5bd8833fb63e2ef4f3264f934c
could use `Txid` for this as well
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1386820528)
aedfef28af6e8c5bd8833fb63e2ef4f3264f934c
could use `Txid` for this as well