💬 jamesob commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1254538118)
@mzumsande Yep, I don't see any reason that couldn't be done.
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1254538118)
@mzumsande Yep, I don't see any reason that couldn't be done.
💬 furszy commented on pull request "test: Restore unlimited timeout in IndexWaitSynced":
(https://github.com/bitcoin/bitcoin/pull/28036#issuecomment-1623813703)
Actually, it is not when the thread never finishes. The thread could have finished, but never set the `m_synced` flag to true. Which would make `BlockUntilSyncedToCurrentChain` never return true. Which actually could happen on all the index sync errors.
(https://github.com/bitcoin/bitcoin/pull/28036#issuecomment-1623813703)
Actually, it is not when the thread never finishes. The thread could have finished, but never set the `m_synced` flag to true. Which would make `BlockUntilSyncedToCurrentChain` never return true. Which actually could happen on all the index sync errors.
💬 instagibbs commented on pull request "[POLICY] Ephemeral anchors":
(https://github.com/bitcoin/bitcoin/pull/26403#issuecomment-1623814942)
Random thought:
OP_TRUE is txid-malleable by block maker. e.g., they could theoretically add in superfluous pushes in the `scriptSig` and this would not fail validation because `CLEANSTACK` is a policy-only rule.
In this particular V3 context, it just means your CPFP could change txid, wouldn't do anything else. Maybe in a more general context if we weren't restricted to V3 topology this would be an issue? In the context of keyless anchors, I'm not sure that will ever make sense anyways.
...
(https://github.com/bitcoin/bitcoin/pull/26403#issuecomment-1623814942)
Random thought:
OP_TRUE is txid-malleable by block maker. e.g., they could theoretically add in superfluous pushes in the `scriptSig` and this would not fail validation because `CLEANSTACK` is a policy-only rule.
In this particular V3 context, it just means your CPFP could change txid, wouldn't do anything else. Maybe in a more general context if we weren't restricted to V3 topology this would be an issue? In the context of keyless anchors, I'm not sure that will ever make sense anyways.
...
💬 ajtowns commented on pull request "test: Restore unlimited timeout in IndexWaitSynced":
(https://github.com/bitcoin/bitcoin/pull/28036#issuecomment-1623825136)
utACK fabed7eb796637c02e3677ebbe183d90b258ba69
Could also restore the original behaviour by doing something like:
```c++
void IndexWaitSynced(BaseIndex& index, std::chrono::microseconds timeout)
{
const auto end = SteadyClock::now() + timeout;
while (!index.BlockUntilSyncedToCurrentChain()) {
Assert(timeout <= 0s || SteadyClock::now < end);
UninterruptibleSleep(100ms);
}
}
```
and calling `IndexWaitSynced(index, 10s)` normally, but `IndexWaitSynced(i
...
(https://github.com/bitcoin/bitcoin/pull/28036#issuecomment-1623825136)
utACK fabed7eb796637c02e3677ebbe183d90b258ba69
Could also restore the original behaviour by doing something like:
```c++
void IndexWaitSynced(BaseIndex& index, std::chrono::microseconds timeout)
{
const auto end = SteadyClock::now() + timeout;
while (!index.BlockUntilSyncedToCurrentChain()) {
Assert(timeout <= 0s || SteadyClock::now < end);
UninterruptibleSleep(100ms);
}
}
```
and calling `IndexWaitSynced(index, 10s)` normally, but `IndexWaitSynced(i
...
💬 ajtowns commented on pull request "test: bugfix, synchronize indexes synchronously":
(https://github.com/bitcoin/bitcoin/pull/28026#issuecomment-1623831541)
> I think I'd prefer that option because then we wouldn't need to add a test-only arg to `BaseIndex::Start`, plus having the same thread structure as in production seems more natural and more robust with respect to possible future changes of the init sequence and unit tests.
I think I (weakly) agree with this -- having the code be non-threaded in tests but threading in the real use seems like an annoying difference.
(https://github.com/bitcoin/bitcoin/pull/28026#issuecomment-1623831541)
> I think I'd prefer that option because then we wouldn't need to add a test-only arg to `BaseIndex::Start`, plus having the same thread structure as in production seems more natural and more robust with respect to possible future changes of the init sequence and unit tests.
I think I (weakly) agree with this -- having the code be non-threaded in tests but threading in the real use seems like an annoying difference.
💬 willcl-ark commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1254429298)
nit: could use brace initialization here
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1254429298)
nit: could use brace initialization here
🤔 willcl-ark reviewed a pull request: "p2p: Diversify automatic outbound connections with respect to networks"
(https://github.com/bitcoin/bitcoin/pull/27213#pullrequestreview-1516531774)
Approach and lightly tested ACK.
Slight tangent, but also slightly related to the discussion on whether IPV4 and IPV6 should be grouped... I noticed during testing that a majority of my network specific connection attempts were to IPV6 addresses, even though I don't have an IPV6 address and can't make connections to it... This appears to be because all networks are set as [Reachable by default ](https://github.com/bitcoin/bitcoin/blob/c325f0fbae2d6472a78be733d499783f8b69d6b8/src/net.h#L152-L1
...
(https://github.com/bitcoin/bitcoin/pull/27213#pullrequestreview-1516531774)
Approach and lightly tested ACK.
Slight tangent, but also slightly related to the discussion on whether IPV4 and IPV6 should be grouped... I noticed during testing that a majority of my network specific connection attempts were to IPV6 addresses, even though I don't have an IPV6 address and can't make connections to it... This appears to be because all networks are set as [Reachable by default ](https://github.com/bitcoin/bitcoin/blob/c325f0fbae2d6472a78be733d499783f8b69d6b8/src/net.h#L152-L1
...
💬 willcl-ark commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1254495003)
The commit message says every ~1min, but this means it will be ~5min via GetExponentialRand I think? This seems to be more in line with timings I have observed during testing, e.g.
```log
2023-07-05T14:04:26Z [net] Making network specific connection to ipv6_addr:8333
2023-07-05T14:15:16Z [net] Making network specific connection to onion_addr.onion:8333
```
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1254495003)
The commit message says every ~1min, but this means it will be ~5min via GetExponentialRand I think? This seems to be more in line with timings I have observed during testing, e.g.
```log
2023-07-05T14:04:26Z [net] Making network specific connection to ipv6_addr:8333
2023-07-05T14:15:16Z [net] Making network specific connection to onion_addr.onion:8333
```
📝 achow101 opened a pull request: "rpc: Drop migratewallet experimental warning"
(https://github.com/bitcoin/bitcoin/pull/28037)
The migration process itself hasn't fundamentally changed since it was added, so I think it's reasonable to say that it is no longer experimental.
(https://github.com/bitcoin/bitcoin/pull/28037)
The migration process itself hasn't fundamentally changed since it was added, so I think it's reasonable to say that it is no longer experimental.
💬 mzumsande commented on pull request "test: Restore unlimited timeout in IndexWaitSynced":
(https://github.com/bitcoin/bitcoin/pull/28036#issuecomment-1623848833)
ACK fabed7eb796637c02e3677ebbe183d90b258ba69
as per https://github.com/bitcoin/bitcoin/pull/28026#pullrequestreview-1514862606 I like this approach more than running the sync synchronously.
> Could also restore the original behaviour by doing something like:
The problem in the first place was that that the tests for txindex and blockfilterindex would have rare intermittent timeouts. So instead of making guesses about the slowest environment the tests could be run in I think it's ok to j
...
(https://github.com/bitcoin/bitcoin/pull/28036#issuecomment-1623848833)
ACK fabed7eb796637c02e3677ebbe183d90b258ba69
as per https://github.com/bitcoin/bitcoin/pull/28026#pullrequestreview-1514862606 I like this approach more than running the sync synchronously.
> Could also restore the original behaviour by doing something like:
The problem in the first place was that that the tests for txindex and blockfilterindex would have rare intermittent timeouts. So instead of making guesses about the slowest environment the tests could be run in I think it's ok to j
...
🤔 furszy reviewed a pull request: "rpc: Drop migratewallet experimental warning"
(https://github.com/bitcoin/bitcoin/pull/28037#pullrequestreview-1516778416)
There is a small bug in the addressbook migration code (fixed by #26836) that we might want to fix before removing the experimental warning.
Probably, I could decouple the fix and test coverage into an standalone PR so we can get more attention there.
(https://github.com/bitcoin/bitcoin/pull/28037#pullrequestreview-1516778416)
There is a small bug in the addressbook migration code (fixed by #26836) that we might want to fix before removing the experimental warning.
Probably, I could decouple the fix and test coverage into an standalone PR so we can get more attention there.
💬 MarcoFalke commented on pull request "test: Restore unlimited timeout in IndexWaitSynced":
(https://github.com/bitcoin/bitcoin/pull/28036#issuecomment-1623858487)
> Which could happen on all the index sync errors.
Good point. Though, that seems like a bigger problem of the unit tests not handling AbortNode/FatalError/StartShutdown at all?
(https://github.com/bitcoin/bitcoin/pull/28036#issuecomment-1623858487)
> Which could happen on all the index sync errors.
Good point. Though, that seems like a bigger problem of the unit tests not handling AbortNode/FatalError/StartShutdown at all?
👍 dergoegge approved a pull request: "fuzz: Generate rpc fuzz targets individually"
(https://github.com/bitcoin/bitcoin/pull/28015#pullrequestreview-1516804537)
ACK fa1e27fe8ec42764d0250c82a83d774c15798c4a
(https://github.com/bitcoin/bitcoin/pull/28015#pullrequestreview-1516804537)
ACK fa1e27fe8ec42764d0250c82a83d774c15798c4a
💬 glozow commented on pull request "Package Relay 1/3: Introduce TxPackageTracker as Orphan Resolution Module":
(https://github.com/bitcoin/bitcoin/pull/28031#issuecomment-1623886758)
Investigating, thanks @DrahtBot
(https://github.com/bitcoin/bitcoin/pull/28031#issuecomment-1623886758)
Investigating, thanks @DrahtBot
💬 furszy commented on pull request "test: Restore unlimited timeout in IndexWaitSynced":
(https://github.com/bitcoin/bitcoin/pull/28036#issuecomment-1623901376)
> > Which could happen on all the index sync errors.
>
> Good point. Though, that seems like a bigger problem of the unit tests not handling AbortNode/FatalError/StartShutdown at all?
The quickest fix would be to add a `ShutdownRequested()` call into the `IndexWaitSynced()` loop. And verify at the end of the function that the index class is synced by calling:
```
BOOST_CHECK(index.GetSummary().synced);
```
Still, the "AbortNode/FatalError/StartShutdown" treatment on the tests topic s
...
(https://github.com/bitcoin/bitcoin/pull/28036#issuecomment-1623901376)
> > Which could happen on all the index sync errors.
>
> Good point. Though, that seems like a bigger problem of the unit tests not handling AbortNode/FatalError/StartShutdown at all?
The quickest fix would be to add a `ShutdownRequested()` call into the `IndexWaitSynced()` loop. And verify at the end of the function that the index class is synced by calling:
```
BOOST_CHECK(index.GetSummary().synced);
```
Still, the "AbortNode/FatalError/StartShutdown" treatment on the tests topic s
...
💬 achow101 commented on pull request "wallet: fix bug, simplify and add coverage for addressbook migration":
(https://github.com/bitcoin/bitcoin/pull/26836#issuecomment-1623901650)
This PR ends up doing a lot of different things that are only tangentially related. I think it could be split up into a few smaller PRs, e.g. one for the migration fix, one for the migration refactoring, and one for the other refactors.
(https://github.com/bitcoin/bitcoin/pull/26836#issuecomment-1623901650)
This PR ends up doing a lot of different things that are only tangentially related. I think it could be split up into a few smaller PRs, e.g. one for the migration fix, one for the migration refactoring, and one for the other refactors.
💬 vasild commented on pull request "test: remove race in the user-agent reception check":
(https://github.com/bitcoin/bitcoin/pull/27986#issuecomment-1623911361)
`28d26c4f37...20b49460b3`: take https://github.com/bitcoin/bitcoin/pull/27986#discussion_r1253688687
Invalidates ACK from @jonatack
(https://github.com/bitcoin/bitcoin/pull/27986#issuecomment-1623911361)
`28d26c4f37...20b49460b3`: take https://github.com/bitcoin/bitcoin/pull/27986#discussion_r1253688687
Invalidates ACK from @jonatack
💬 vasild commented on pull request "test: remove race in the user-agent reception check":
(https://github.com/bitcoin/bitcoin/pull/27986#discussion_r1254628447)
Looks better, thanks!
(https://github.com/bitcoin/bitcoin/pull/27986#discussion_r1254628447)
Looks better, thanks!
🤔 furszy reviewed a pull request: "wallet: fix bug, simplify and add coverage for addressbook migration"
(https://github.com/bitcoin/bitcoin/pull/26836#pullrequestreview-1516852781)
> This PR ends up doing a lot of different things that are only tangentially related. I think it could be split up into a few smaller PRs, e.g. one for the migration fix, one for the migration refactoring, and one for the other refactors.
sure, will work on it.
(https://github.com/bitcoin/bitcoin/pull/26836#pullrequestreview-1516852781)
> This PR ends up doing a lot of different things that are only tangentially related. I think it could be split up into a few smaller PRs, e.g. one for the migration fix, one for the migration refactoring, and one for the other refactors.
sure, will work on it.
🤔 instagibbs reviewed a pull request: "Package Relay 1/3: Introduce TxPackageTracker as Orphan Resolution Module"
(https://github.com/bitcoin/bitcoin/pull/28031#pullrequestreview-1515046495)
some initial comments through https://github.com/bitcoin/bitcoin/pull/28031/commits/116378efc1c9c1fe0d26cb42e2bdbb5770815c35
Log changes suggested are helpful for tracing what's happening in the orphanage on my node I'm testing.
(https://github.com/bitcoin/bitcoin/pull/28031#pullrequestreview-1515046495)
some initial comments through https://github.com/bitcoin/bitcoin/pull/28031/commits/116378efc1c9c1fe0d26cb42e2bdbb5770815c35
Log changes suggested are helpful for tracing what's happening in the orphanage on my node I'm testing.