✅ benthecarman closed a pull request: "Allow accepting non-standard transactions on mainnet"
(https://github.com/bitcoin/bitcoin/pull/27578)
(https://github.com/bitcoin/bitcoin/pull/27578)
💬 fanquake commented on pull request "refactor: Remove need to pass chainparams to BlockManager methods":
(https://github.com/bitcoin/bitcoin/pull/27570#issuecomment-1536482035)
cc @jamesob, this is going to conflict with #15606.
(https://github.com/bitcoin/bitcoin/pull/27570#issuecomment-1536482035)
cc @jamesob, this is going to conflict with #15606.
💬 jamesob commented on pull request "refactor: Remove need to pass chainparams to BlockManager methods":
(https://github.com/bitcoin/bitcoin/pull/27570#issuecomment-1536490676)
Is this some kind of dependency for libbitcoinkernel or for something other than general tidy-up impulses? If so, feel free to merge - assumeutxo is fresh in my brain and handling the rebase won't be so painful as a result.
Otherwise, wouldn't mind this getting shelved for later. But really no big deal either way. I'd say if you're going to merge do it soon so that I can manage the rebase with fresh context.
(https://github.com/bitcoin/bitcoin/pull/27570#issuecomment-1536490676)
Is this some kind of dependency for libbitcoinkernel or for something other than general tidy-up impulses? If so, feel free to merge - assumeutxo is fresh in my brain and handling the rebase won't be so painful as a result.
Otherwise, wouldn't mind this getting shelved for later. But really no big deal either way. I'd say if you're going to merge do it soon so that I can manage the rebase with fresh context.
💬 fanquake commented on pull request "refactor: Remove need to pass chainparams to BlockManager methods":
(https://github.com/bitcoin/bitcoin/pull/27570#issuecomment-1536499967)
> Is this some kind of dependency for libbitcoinkernel or for something other than general tidy-up impulses?
It spawned out of here: https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1183556059. If you're happy to rebase over then I'm just going to go-ahead and merge this now. @TheCharlatan can then address any of his own comments as part of #27125 (which is also going to conflict with 15606, so probably worth having a look there as well.)
(https://github.com/bitcoin/bitcoin/pull/27570#issuecomment-1536499967)
> Is this some kind of dependency for libbitcoinkernel or for something other than general tidy-up impulses?
It spawned out of here: https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1183556059. If you're happy to rebase over then I'm just going to go-ahead and merge this now. @TheCharlatan can then address any of his own comments as part of #27125 (which is also going to conflict with 15606, so probably worth having a look there as well.)
🚀 fanquake merged a pull request: "refactor: Remove need to pass chainparams to BlockManager methods"
(https://github.com/bitcoin/bitcoin/pull/27570)
(https://github.com/bitcoin/bitcoin/pull/27570)
💬 hebasto commented on pull request "test: Explicitly specify directory where to search tests for":
(https://github.com/bitcoin/bitcoin/pull/27561#issuecomment-1536525721)
@stickies-v
> Would it be possible to provide instructions on replicating how to make this fail without`sys.path.append(tests_dir)`?
Sure. I've updated the PR description with steps to replicate the failure.
(https://github.com/bitcoin/bitcoin/pull/27561#issuecomment-1536525721)
@stickies-v
> Would it be possible to provide instructions on replicating how to make this fail without`sys.path.append(tests_dir)`?
Sure. I've updated the PR description with steps to replicate the failure.
💬 hebasto commented on pull request "test: Explicitly specify directory where to search tests for":
(https://github.com/bitcoin/bitcoin/pull/27561#issuecomment-1536535154)
Updated a3108880fac89d4472fe3b24d133e9645bed41cf -> fdff2156e1ebc11818f34759bff80e7d37dfff7d ([pr27561.01](https://github.com/hebasto/bitcoin/commits/pr27561.01) -> [pr27561.02](https://github.com/hebasto/bitcoin/commits/pr27561.02), [diff](https://github.com/hebasto/bitcoin/compare/pr27561.01..pr27561.02)):
- addressed @stickies-v's [comment](https://github.com/bitcoin/bitcoin/pull/27561#discussion_r1186191720)
(https://github.com/bitcoin/bitcoin/pull/27561#issuecomment-1536535154)
Updated a3108880fac89d4472fe3b24d133e9645bed41cf -> fdff2156e1ebc11818f34759bff80e7d37dfff7d ([pr27561.01](https://github.com/hebasto/bitcoin/commits/pr27561.01) -> [pr27561.02](https://github.com/hebasto/bitcoin/commits/pr27561.02), [diff](https://github.com/hebasto/bitcoin/compare/pr27561.01..pr27561.02)):
- addressed @stickies-v's [comment](https://github.com/bitcoin/bitcoin/pull/27561#discussion_r1186191720)
💬 hebasto commented on pull request "test: Explicitly specify directory where to search tests for":
(https://github.com/bitcoin/bitcoin/pull/27561#discussion_r1186317263)
Thanks! [Updated](https://github.com/bitcoin/bitcoin/pull/27561#issuecomment-1536535154).
(https://github.com/bitcoin/bitcoin/pull/27561#discussion_r1186317263)
Thanks! [Updated](https://github.com/bitcoin/bitcoin/pull/27561#issuecomment-1536535154).
💬 pinheadmz commented on pull request "net: call getaddrinfo() in detachable thread to prevent stalling":
(https://github.com/bitcoin/bitcoin/pull/27557#issuecomment-1536552393)
added call to `thread.join()` in the "happy path" so the only threads we should be abandoning are those that time out. Also setup the unit test to stub in a black hole for `getaddressinfo` to ensure the 2-second timeout is enforced. On my machine at least with thread and memory sanitizers I'm not getting any errors for the abandoned thread. I'm going to try some more experiments with a mainnet node as well.
(https://github.com/bitcoin/bitcoin/pull/27557#issuecomment-1536552393)
added call to `thread.join()` in the "happy path" so the only threads we should be abandoning are those that time out. Also setup the unit test to stub in a black hole for `getaddressinfo` to ensure the 2-second timeout is enforced. On my machine at least with thread and memory sanitizers I'm not getting any errors for the abandoned thread. I'm going to try some more experiments with a mainnet node as well.
💬 glozow commented on pull request "Allow accepting non-standard transactions on mainnet":
(https://github.com/bitcoin/bitcoin/pull/27578#issuecomment-1536583890)
I agree that the network functions best when everybody hears about transactions before they are mined, but we should never _assume_ that mempool policies are homogeneous. I also believe that policy has room for improvement and we should change rules if they are harmful and unhelpful. Miners accepting out-of-band nonstandard transactions can be a symptom of Bitcoin Core's rules not being accommodating enough. (fwiw it can also mean that people are doing kind of spammy things).
However, I haven
...
(https://github.com/bitcoin/bitcoin/pull/27578#issuecomment-1536583890)
I agree that the network functions best when everybody hears about transactions before they are mined, but we should never _assume_ that mempool policies are homogeneous. I also believe that policy has room for improvement and we should change rules if they are harmful and unhelpful. Miners accepting out-of-band nonstandard transactions can be a symptom of Bitcoin Core's rules not being accommodating enough. (fwiw it can also mean that people are doing kind of spammy things).
However, I haven
...
📝 brunoerg opened a pull request: "fuzz: improve `coinselection`"
(https://github.com/bitcoin/bitcoin/pull/27585)
This PR:
- Moves coin creation to its own function called `CreateCoins`.
- Add coverage for `EligibleForSpending`
- Add coverage for `AddInputs`: get result of each algorithm (srd, knapsack and bnb), call `CreateCoins` and add into them.
- Add coverage for `GetShuffledInputVector` and `GetInputSet` using the result of each algorithm (srd, knapsack and bnb).
- Add coverage for `Merge`: Call SRD with the new utxos and, if successful, try to merge with the previous SRD result.
(https://github.com/bitcoin/bitcoin/pull/27585)
This PR:
- Moves coin creation to its own function called `CreateCoins`.
- Add coverage for `EligibleForSpending`
- Add coverage for `AddInputs`: get result of each algorithm (srd, knapsack and bnb), call `CreateCoins` and add into them.
- Add coverage for `GetShuffledInputVector` and `GetInputSet` using the result of each algorithm (srd, knapsack and bnb).
- Add coverage for `Merge`: Call SRD with the new utxos and, if successful, try to merge with the previous SRD result.
💬 Sjors commented on pull request "assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/15606#issuecomment-1536606404)
Here's a [torrent](magnet:?xt=urn:btih:a457a54c76dfdbb3f44e3485a84c4772bea647e0&dn=utxo-788000.dat&tr=udp%3A%2F%2Ftracker.bitcoin.sprovoost.nl%3A6969) for the snapshot.
(https://github.com/bitcoin/bitcoin/pull/15606#issuecomment-1536606404)
Here's a [torrent](magnet:?xt=urn:btih:a457a54c76dfdbb3f44e3485a84c4772bea647e0&dn=utxo-788000.dat&tr=udp%3A%2F%2Ftracker.bitcoin.sprovoost.nl%3A6969) for the snapshot.
🤔 jonatack reviewed a pull request: "p2p: Allow whitelisting outgoing connections"
(https://github.com/bitcoin/bitcoin/pull/27114#pullrequestreview-1415072190)
ACK 19f3f3a316660890f03d6ddcb4a4230eb2ff5e91 with some suggestions
(https://github.com/bitcoin/bitcoin/pull/27114#pullrequestreview-1415072190)
ACK 19f3f3a316660890f03d6ddcb4a4230eb2ff5e91 with some suggestions
💬 jonatack commented on pull request "p2p: Allow whitelisting outgoing connections":
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1186325710)
https://github.com/bitcoin/bitcoin/commit/f52fc86d0840221a5d106eec201a4dcffd7d5774 can make the function `static` while touching this line
```suggestion
static bool TryParsePermissionFlags(const std::string& str, NetPermissionFlags& output, ConnectionDirection* output_connection_direction, size_t& readen, bilingual_str& error)
```
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1186325710)
https://github.com/bitcoin/bitcoin/commit/f52fc86d0840221a5d106eec201a4dcffd7d5774 can make the function `static` while touching this line
```suggestion
static bool TryParsePermissionFlags(const std::string& str, NetPermissionFlags& output, ConnectionDirection* output_connection_direction, size_t& readen, bilingual_str& error)
```
💬 jonatack commented on pull request "p2p: Allow whitelisting outgoing connections":
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1186270266)
39f6466 could be wrong, but there doesn't seem to be any need for this to be `CConnMan` class member instead of a static helper function in the implementation file.
<details><summary>sample diff, modulo splitting the changes between the first two commits</summary><p>
```diff
diff --git a/src/net.cpp b/src/net.cpp
index dc2fa7d2e17..7fd964b8cc3 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -360,6 +360,20 @@ bool IsLocal(const CService& addr)
return mapLocalHost.count(addr) > 0;
...
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1186270266)
39f6466 could be wrong, but there doesn't seem to be any need for this to be `CConnMan` class member instead of a static helper function in the implementation file.
<details><summary>sample diff, modulo splitting the changes between the first two commits</summary><p>
```diff
diff --git a/src/net.cpp b/src/net.cpp
index dc2fa7d2e17..7fd964b8cc3 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -360,6 +360,20 @@ bool IsLocal(const CService& addr)
return mapLocalHost.count(addr) > 0;
...
💬 jonatack commented on pull request "p2p: Allow whitelisting outgoing connections":
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1186352750)
https://github.com/bitcoin/bitcoin/commit/f52fc86d0840221a5d106eec201a4dcffd7d5774 missing header, sort
```diff
#include <net_permissions.h>
+#include <netbase.h>
#include <test/fuzz/FuzzedDataProvider.h>
@@ -31,8 +32,8 @@ FUZZ_TARGET(net_permissions)
NetWhitelistPermissions net_whitelist_permissions;
- bilingual_str error_net_whitelist_permissions;
ConnectionDirection connection_direction;
+ bilingual_str error_net_whitelist_permissions;
if (NetWhitelistPe
...
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1186352750)
https://github.com/bitcoin/bitcoin/commit/f52fc86d0840221a5d106eec201a4dcffd7d5774 missing header, sort
```diff
#include <net_permissions.h>
+#include <netbase.h>
#include <test/fuzz/FuzzedDataProvider.h>
@@ -31,8 +32,8 @@ FUZZ_TARGET(net_permissions)
NetWhitelistPermissions net_whitelist_permissions;
- bilingual_str error_net_whitelist_permissions;
ConnectionDirection connection_direction;
+ bilingual_str error_net_whitelist_permissions;
if (NetWhitelistPe
...
💬 jonatack commented on pull request "p2p: Allow whitelisting outgoing connections":
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1186367052)
https://github.com/bitcoin/bitcoin/commit/f52fc86d0840221a5d106eec201a4dcffd7d5774 Can also test the error case.
```diff
@@ -446,6 +446,9 @@ BOOST_AUTO_TEST_CASE(netpermissions_test)
BOOST_CHECK(NetWhitebindPermissions::TryParse(",,@1.2.3.4:32", whitebindPermissions, error));
BOOST_CHECK_EQUAL(whitebindPermissions.m_flags, NetPermissionFlags::None);
+ BOOST_CHECK(!NetWhitebindPermissions::TryParse("out,forcerelay@1.2.3.4:32", whitebindPermissions, error));
+ BOOST_CHEC
...
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1186367052)
https://github.com/bitcoin/bitcoin/commit/f52fc86d0840221a5d106eec201a4dcffd7d5774 Can also test the error case.
```diff
@@ -446,6 +446,9 @@ BOOST_AUTO_TEST_CASE(netpermissions_test)
BOOST_CHECK(NetWhitebindPermissions::TryParse(",,@1.2.3.4:32", whitebindPermissions, error));
BOOST_CHECK_EQUAL(whitebindPermissions.m_flags, NetPermissionFlags::None);
+ BOOST_CHECK(!NetWhitebindPermissions::TryParse("out,forcerelay@1.2.3.4:32", whitebindPermissions, error));
+ BOOST_CHEC
...
💬 jonatack commented on pull request "p2p: Allow whitelisting outgoing connections":
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1186336634)
https://github.com/bitcoin/bitcoin/commit/f52fc86d0840221a5d106eec201a4dcffd7d5774
- The following error message would be clearer, I think
```suggestion
error = _("whitebind may only be used for incoming connections (\"out\" was passed)");
```
- My first thought was if it would make sense to pass `output_connection_direction` as a `std::optional<ConnectionDirection>`.
- But also, relying on `nullptr` and overloading this param for the callee to know who the ca
...
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1186336634)
https://github.com/bitcoin/bitcoin/commit/f52fc86d0840221a5d106eec201a4dcffd7d5774
- The following error message would be clearer, I think
```suggestion
error = _("whitebind may only be used for incoming connections (\"out\" was passed)");
```
- My first thought was if it would make sense to pass `output_connection_direction` as a `std::optional<ConnectionDirection>`.
- But also, relying on `nullptr` and overloading this param for the callee to know who the ca
...
💬 jonatack commented on pull request "p2p: Allow whitelisting outgoing connections":
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1186324954)
f52fc86 named arg, if you retouch and stay with the nullptr approach
```suggestion
if (!TryParsePermissionFlags(str, flags, /*output_connection_direction=*/nullptr, offset, error)) return false;
```
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1186324954)
f52fc86 named arg, if you retouch and stay with the nullptr approach
```suggestion
if (!TryParsePermissionFlags(str, flags, /*output_connection_direction=*/nullptr, offset, error)) return false;
```
💬 jonatack commented on pull request "p2p: Allow whitelisting outgoing connections":
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1186377787)
19f3f3a A comment here for future readers documenting when/why to use this would be good.
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1186377787)
19f3f3a A comment here for future readers documenting when/why to use this would be good.