💬 theStack commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1578122793)
refactoring nit to avoid multiple accesses in `cpfp_candidates_different_peer` (feel free to ignore):
```suggestion
const auto [candidate_child, candidate_peer] = cpfp_candidates_different_peer.at(index);
const Package maybe_cpfp_package{ptx, candidate_child};
if (!m_recent_rejects_reconsiderable.contains(GetPackageHash(maybe_cpfp_package))) {
tx_orphan = candidate_child;
orphan_sender = candidate_peer;
b
...
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1578122793)
refactoring nit to avoid multiple accesses in `cpfp_candidates_different_peer` (feel free to ignore):
```suggestion
const auto [candidate_child, candidate_peer] = cpfp_candidates_different_peer.at(index);
const Package maybe_cpfp_package{ptx, candidate_child};
if (!m_recent_rejects_reconsiderable.contains(GetPackageHash(maybe_cpfp_package))) {
tx_orphan = candidate_child;
orphan_sender = candidate_peer;
b
...
💬 achow101 commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1578041562)
In 3dff8f0490b4b92c4adf229a828063dfda6d3a80 "[test] Create coinselection_tests"
Since the expected pattern is to always `GroupCoins(utxo_pool)`, it might make sense to make `utxo_pool` a `std::vector<OutputGroup>` and combine this function with `AddCoins`.
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1578041562)
In 3dff8f0490b4b92c4adf229a828063dfda6d3a80 "[test] Create coinselection_tests"
Since the expected pattern is to always `GroupCoins(utxo_pool)`, it might make sense to make `utxo_pool` a `std::vector<OutputGroup>` and combine this function with `AddCoins`.
💬 achow101 commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1578039066)
In 3dff8f0490b4b92c4adf229a828063dfda6d3a80 "[test] Create coinselection_tests"
There doesn't seem to be a purpose for having a static variable here. We basically treat it as temporary since it's cleared every time.
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1578039066)
In 3dff8f0490b4b92c4adf229a828063dfda6d3a80 "[test] Create coinselection_tests"
There doesn't seem to be a purpose for having a static variable here. We basically treat it as temporary since it's cleared every time.
💬 achow101 commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1578036822)
In 3dff8f0490b4b92c4adf229a828063dfda6d3a80 "[test] Create coinselection_tests"
The elements in `coins` are `CAmount`, not `int`.
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1578036822)
In 3dff8f0490b4b92c4adf229a828063dfda6d3a80 "[test] Create coinselection_tests"
The elements in `coins` are `CAmount`, not `int`.
💬 achow101 commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1578043081)
In e286c6470bbaa1358fecb7c5a7e109583d158780 "[test] Recreate simple BnB success tests"
The elements of `expected_input_amounts` are `CAmount` not `int`.
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1578043081)
In e286c6470bbaa1358fecb7c5a7e109583d158780 "[test] Recreate simple BnB success tests"
The elements of `expected_input_amounts` are `CAmount` not `int`.
💬 achow101 commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1578048284)
In e286c6470bbaa1358fecb7c5a7e109583d158780 "[test] Recreate simple BnB success tests"
The `*_MESSAGE` functions print out the message on failure, not on success. So the message should be something that better indicates what the failure was, not something that might suggest something was successful.
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1578048284)
In e286c6470bbaa1358fecb7c5a7e109583d158780 "[test] Recreate simple BnB success tests"
The `*_MESSAGE` functions print out the message on failure, not on success. So the message should be something that better indicates what the failure was, not something that might suggest something was successful.
💬 brunoerg commented on pull request "fuzz: wallet: add target for `CreateTransaction`":
(https://github.com/bitcoin/bitcoin/pull/29936#discussion_r1578189219)
It seems the addition of a new util file makes this PR conflicts with other ones. Perhaps it's better to put everything in `wallet/test/util` or leave as is? What do you think @maflcko?
(https://github.com/bitcoin/bitcoin/pull/29936#discussion_r1578189219)
It seems the addition of a new util file makes this PR conflicts with other ones. Perhaps it's better to put everything in `wallet/test/util` or leave as is? What do you think @maflcko?
💬 hebasto commented on pull request "Reintroduce external signer support for Windows":
(https://github.com/bitcoin/bitcoin/pull/29868#issuecomment-2075383041)
Rebased on top of the merged #29910.
(https://github.com/bitcoin/bitcoin/pull/29868#issuecomment-2075383041)
Rebased on top of the merged #29910.
💬 glozow commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1578202506)
Yeah that's a typo :+1:
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1578202506)
Yeah that's a typo :+1:
💬 glozow commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1578202070)
I think we'd want to make sure that even if we had a bad `senders` vector we would still call `ProcessValidTx` / `ProcessInvalidTx` (seems kind of bad not to). Maybe a better approach is to use -1 when we don't know what it is - I do realize we're not handling the empty vector case.
(Yeah maybe this is moot as the 1 caller should always make `senders` properly...)
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1578202070)
I think we'd want to make sure that even if we had a bad `senders` vector we would still call `ProcessValidTx` / `ProcessInvalidTx` (seems kind of bad not to). Maybe a better approach is to use -1 when we don't know what it is - I do realize we're not handling the empty vector case.
(Yeah maybe this is moot as the 1 caller should always make `senders` properly...)
💬 maflcko commented on pull request "fuzz: wallet: add target for `CreateTransaction`":
(https://github.com/bitcoin/bitcoin/pull/29936#discussion_r1578211737)
Actually I don't understand why the last push compiles. CI says:
* https://cirrus-ci.com/task/6011306526900224?logs=ci#L2291 ` CXX test/fuzz/util/libtest_fuzz_a-wallet.o`
* but the wallet is disabled: ` with wallet = no` https://cirrus-ci.com/task/6011306526900224?logs=ci#L1385
Shouldn't this result in a link or even compile error?
(https://github.com/bitcoin/bitcoin/pull/29936#discussion_r1578211737)
Actually I don't understand why the last push compiles. CI says:
* https://cirrus-ci.com/task/6011306526900224?logs=ci#L2291 ` CXX test/fuzz/util/libtest_fuzz_a-wallet.o`
* but the wallet is disabled: ` with wallet = no` https://cirrus-ci.com/task/6011306526900224?logs=ci#L1385
Shouldn't this result in a link or even compile error?
👍 vasild approved a pull request: "doc: Bash is needed in gen_id and is not installed on FreeBSD by default"
(https://github.com/bitcoin/bitcoin/pull/29953#pullrequestreview-2020413434)
ACK 9381052194a78024b3994cc6ad906858c477b88f
(https://github.com/bitcoin/bitcoin/pull/29953#pullrequestreview-2020413434)
ACK 9381052194a78024b3994cc6ad906858c477b88f
💬 m3dwards commented on pull request "depends: build libnatpmp with CMake":
(https://github.com/bitcoin/bitcoin/pull/29708#discussion_r1578213646)
Perhaps this is what we want but I thought it worth mentioning that there is a difference on my Mac between this and having cmake call make with: `cmake --build . --target natpmp`.
$(MAKE) from autotools for me calls GNU MAKE 3.81 and cmake delegates to gmake being GNU MAKE 4.4.1.
Should we call cmake instead and allow it to decide the best make to delegate to?
(https://github.com/bitcoin/bitcoin/pull/29708#discussion_r1578213646)
Perhaps this is what we want but I thought it worth mentioning that there is a difference on my Mac between this and having cmake call make with: `cmake --build . --target natpmp`.
$(MAKE) from autotools for me calls GNU MAKE 3.81 and cmake delegates to gmake being GNU MAKE 4.4.1.
Should we call cmake instead and allow it to decide the best make to delegate to?
💬 fanquake commented on pull request "depends: build libnatpmp with CMake":
(https://github.com/bitcoin/bitcoin/pull/29708#discussion_r1578220994)
Possibly, but if we are going to do that, we could do it globally/all at once, or even set something at a higher level in depends. Not sure if we have to change this during these migrations.
(https://github.com/bitcoin/bitcoin/pull/29708#discussion_r1578220994)
Possibly, but if we are going to do that, we could do it globally/all at once, or even set something at a higher level in depends. Not sure if we have to change this during these migrations.
💬 brunoerg commented on pull request "fuzz: wallet: add target for `CreateTransaction`":
(https://github.com/bitcoin/bitcoin/pull/29936#discussion_r1578242552)
You're right. I just tested here without wallet and it compiles. We don't have any verification for `ENABLE_WALLET` in `Makefile.test_fuzz.include` like in `Makefile.test.include`. I could include it.
(https://github.com/bitcoin/bitcoin/pull/29936#discussion_r1578242552)
You're right. I just tested here without wallet and it compiles. We don't have any verification for `ENABLE_WALLET` in `Makefile.test_fuzz.include` like in `Makefile.test.include`. I could include it.
💬 mzumsande commented on issue "Manually Banning Peers Results in Crash":
(https://github.com/bitcoin/bitcoin/issues/29916#issuecomment-2075482741)
> Hate to be the deliverer of bad news but I am also experiencing this issue on my Ubuntu installations after extensive testing.
I just tried in out under ubuntu, and banning both single and multiple peers worked fine.
What do you mean with "extensive testing"? If it happens always, just starting the program and banning a peer should be enough? Or does it happen only sometimes for you?
(https://github.com/bitcoin/bitcoin/issues/29916#issuecomment-2075482741)
> Hate to be the deliverer of bad news but I am also experiencing this issue on my Ubuntu installations after extensive testing.
I just tried in out under ubuntu, and banning both single and multiple peers worked fine.
What do you mean with "extensive testing"? If it happens always, just starting the program and banning a peer should be enough? Or does it happen only sometimes for you?
💬 brunoerg commented on pull request "fuzz: wallet: add target for `CreateTransaction`":
(https://github.com/bitcoin/bitcoin/pull/29936#discussion_r1578283925)
```diff
diff --git a/src/Makefile.test_fuzz.include b/src/Makefile.test_fuzz.include
index 3f6cc30dc4..28f05c0968 100644
--- a/src/Makefile.test_fuzz.include
+++ b/src/Makefile.test_fuzz.include
@@ -13,8 +13,7 @@ TEST_FUZZ_H = \
test/fuzz/util.h \
test/fuzz/util/descriptor.h \
test/fuzz/util/mempool.h \
- test/fuzz/util/net.h \
- test/fuzz/util/wallet.h
+ test/fuzz/util/net.h
libtest_fuzz_a_CPPFLAGS = $(AM_CPPFLAGS) $(BITCOIN_INCLUDES) $(BOOST_CPPFLAGS)
...
(https://github.com/bitcoin/bitcoin/pull/29936#discussion_r1578283925)
```diff
diff --git a/src/Makefile.test_fuzz.include b/src/Makefile.test_fuzz.include
index 3f6cc30dc4..28f05c0968 100644
--- a/src/Makefile.test_fuzz.include
+++ b/src/Makefile.test_fuzz.include
@@ -13,8 +13,7 @@ TEST_FUZZ_H = \
test/fuzz/util.h \
test/fuzz/util/descriptor.h \
test/fuzz/util/mempool.h \
- test/fuzz/util/net.h \
- test/fuzz/util/wallet.h
+ test/fuzz/util/net.h
libtest_fuzz_a_CPPFLAGS = $(AM_CPPFLAGS) $(BITCOIN_INCLUDES) $(BOOST_CPPFLAGS)
...
🤔 achow101 reviewed a pull request: "Fix waste calculation in SelectionResult"
(https://github.com/bitcoin/bitcoin/pull/28366#pullrequestreview-2020564522)
ACK e95b9159380f2de7f9a6e7a202cc171ad285ee6c
(https://github.com/bitcoin/bitcoin/pull/28366#pullrequestreview-2020564522)
ACK e95b9159380f2de7f9a6e7a202cc171ad285ee6c
💬 achow101 commented on pull request "Fix waste calculation in SelectionResult":
(https://github.com/bitcoin/bitcoin/pull/28366#discussion_r1578299095)
In 44db79b0c48d6b1a26dba6ab01c2a296d610c06b "Refactor ComputeAndSetWaste()"
nit: Unnecessary parentheses, also usually the check of the value is after the function. Could also use brace initialization.
```suggestion
bool makes_change{GetChange(min_viable_change, change_fee) != 0};
```
(https://github.com/bitcoin/bitcoin/pull/28366#discussion_r1578299095)
In 44db79b0c48d6b1a26dba6ab01c2a296d610c06b "Refactor ComputeAndSetWaste()"
nit: Unnecessary parentheses, also usually the check of the value is after the function. Could also use brace initialization.
```suggestion
bool makes_change{GetChange(min_viable_change, change_fee) != 0};
```
✅ achow101 closed a pull request: "Add Gutter Guard Selector"
(https://github.com/bitcoin/bitcoin/pull/28977)
(https://github.com/bitcoin/bitcoin/pull/28977)