💬 vasild commented on pull request "net: replace manual reference counting of CNode with shared_ptr":
(https://github.com/bitcoin/bitcoin/pull/32015#discussion_r2048782819)
_Bringing the anti-`shared_ptr` discussion to its own dedicated thread here._
> https://github.com/bitcoin/bitcoin/pull/32015#issuecomment-2711463453
> Instead of passing around shared pointers to mutable data (aka global variables in disguise), would it be possible to increase the level of abstraction and pass around values with regular semantics?
> https://github.com/bitcoin/bitcoin/pull/32015#issuecomment-2808714778
> I used to appreciate `shared_ptr`-like constructs, but as @purpleKa
...
(https://github.com/bitcoin/bitcoin/pull/32015#discussion_r2048782819)
_Bringing the anti-`shared_ptr` discussion to its own dedicated thread here._
> https://github.com/bitcoin/bitcoin/pull/32015#issuecomment-2711463453
> Instead of passing around shared pointers to mutable data (aka global variables in disguise), would it be possible to increase the level of abstraction and pass around values with regular semantics?
> https://github.com/bitcoin/bitcoin/pull/32015#issuecomment-2808714778
> I used to appreciate `shared_ptr`-like constructs, but as @purpleKa
...
💬 fanquake commented on pull request "ci: Add workaround for vcpkg's `libevent` package":
(https://github.com/bitcoin/bitcoin/pull/32184#issuecomment-2812628615)
Backported in #32292.
(https://github.com/bitcoin/bitcoin/pull/32184#issuecomment-2812628615)
Backported in #32292.
🤔 TheCharlatan reviewed a pull request: "[IBD] specialize block serialization"
(https://github.com/bitcoin/bitcoin/pull/31868#pullrequestreview-2775072131)
This looks good, but I am not really reproducing any of the performance changes on my machine yet. Maybe my laptop is just too unreliable though.
(https://github.com/bitcoin/bitcoin/pull/31868#pullrequestreview-2775072131)
This looks good, but I am not really reproducing any of the performance changes on my machine yet. Maybe my laptop is just too unreliable though.
💬 TheCharlatan commented on pull request "[IBD] specialize block serialization":
(https://github.com/bitcoin/bitcoin/pull/31868#discussion_r2048635108)
Nit: Maybe add a description here too along the lines of:
Check if type contains a SizeComputer by seeing if the return type of T's GetStream() method is a SizeComputer.
(https://github.com/bitcoin/bitcoin/pull/31868#discussion_r2048635108)
Nit: Maybe add a description here too along the lines of:
Check if type contains a SizeComputer by seeing if the return type of T's GetStream() method is a SizeComputer.
💬 TheCharlatan commented on pull request "[IBD] specialize block serialization":
(https://github.com/bitcoin/bitcoin/pull/31868#discussion_r2048540359)
In commit c9a69f9088340df88017752e1016670141b6ad74:
Looking at the concept, could this also be `unsigned char`?
(https://github.com/bitcoin/bitcoin/pull/31868#discussion_r2048540359)
In commit c9a69f9088340df88017752e1016670141b6ad74:
Looking at the concept, could this also be `unsigned char`?
📝 fanquake converted_to_draft a pull request: "docs: improve development guidelines for PR merging"
(https://github.com/bitcoin/bitcoin/pull/32006)
Reword development guideline of PR merging to be more general meaning, expanding the meaning beyond CI
Based on the [comment](https://github.com/bitcoin/bitcoin/pull/32006#discussion_r1998121846) below
(https://github.com/bitcoin/bitcoin/pull/32006)
Reword development guideline of PR merging to be more general meaning, expanding the meaning beyond CI
Based on the [comment](https://github.com/bitcoin/bitcoin/pull/32006#discussion_r1998121846) below
👍 dergoegge approved a pull request: "fuzz: Avoid integer sanitizer warnings in policy_estimator target"
(https://github.com/bitcoin/bitcoin/pull/32154#pullrequestreview-2775513656)
utACK fa6a007b8e7b68d559b30c04dd8d76c877bef133
(https://github.com/bitcoin/bitcoin/pull/32154#pullrequestreview-2775513656)
utACK fa6a007b8e7b68d559b30c04dd8d76c877bef133
💬 fanquake commented on pull request "docs: improve development guidelines for PR merging":
(https://github.com/bitcoin/bitcoin/pull/32006#issuecomment-2812661425)
@suiyuan1314 what's the status of this? Concept ~0.
(https://github.com/bitcoin/bitcoin/pull/32006#issuecomment-2812661425)
@suiyuan1314 what's the status of this? Concept ~0.
💬 fanquake commented on pull request "fuzz: enable running fuzz test cases in Debug mode":
(https://github.com/bitcoin/bitcoin/pull/32113#issuecomment-2812663946)
cc @dergoegge
(https://github.com/bitcoin/bitcoin/pull/32113#issuecomment-2812663946)
cc @dergoegge
🤔 l0rinc requested changes to a pull request: "util: explicitly close all AutoFiles that have been written"
(https://github.com/bitcoin/bitcoin/pull/29307#pullrequestreview-2775379592)
Concept ACK, we definitely want to know if this happens!
* It seems we're using the return value as a boolean - can make make it return one?
* The error messages seem all over the place, can we unify them to `LogError`?
* Not sure if scope-ing the AutoFile + closing it explicitly is the best solution - would probably prefer @ryanofsky's `std::function<void()> on_error` or @maflcko's recommendation to let `AutoFile` guarantee proper closing.
* Sometimes we're explicitly showing a closing r
...
(https://github.com/bitcoin/bitcoin/pull/29307#pullrequestreview-2775379592)
Concept ACK, we definitely want to know if this happens!
* It seems we're using the return value as a boolean - can make make it return one?
* The error messages seem all over the place, can we unify them to `LogError`?
* Not sure if scope-ing the AutoFile + closing it explicitly is the best solution - would probably prefer @ryanofsky's `std::function<void()> on_error` or @maflcko's recommendation to let `AutoFile` guarantee proper closing.
* Sometimes we're explicitly showing a closing r
...
💬 l0rinc commented on pull request "util: explicitly close all AutoFiles that have been written":
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r2048754753)
I'd either delete this now or put it before the actual closing. And do we really need to make this unlikely scenario occupy so much space? And in the other case we simply used `LogError` instead:
```C++
// Make sure that the file is closed before we call `FlushUndoFile`.
if (file.fclose()) {
LogError("Failed to close block undo file %s: %s", pos.ToString(), SysErrorString(errno));
return FatalError(m_opts.notifications, state, _("Failed to close block undo file."));
}
```
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r2048754753)
I'd either delete this now or put it before the actual closing. And do we really need to make this unlikely scenario occupy so much space? And in the other case we simply used `LogError` instead:
```C++
// Make sure that the file is closed before we call `FlushUndoFile`.
if (file.fclose()) {
LogError("Failed to close block undo file %s: %s", pos.ToString(), SysErrorString(errno));
return FatalError(m_opts.notifications, state, _("Failed to close block undo file."));
}
```
💬 l0rinc commented on pull request "util: explicitly close all AutoFiles that have been written":
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r2048761555)
nit:
```suggestion
throw std::runtime_error{strprintf("Error closing XOR key file %s: %s",
```
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r2048761555)
nit:
```suggestion
throw std::runtime_error{strprintf("Error closing XOR key file %s: %s",
```
💬 l0rinc commented on pull request "util: explicitly close all AutoFiles that have been written":
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r2048729097)
If we can't fully commit to disk, we won't call `fclose` directly - we'll rely on the destructor's close.
We'd get a commitment failure message (though that could have been successful) - and maybe another message from the destructor.
Could we separate the two errors?
```C++
if (!file.Commit()) {
LogError("%s: Failed to commit filter file %d", __func__, pos.nFile);
return false;
}
if (file.fclose()) {
LogError("%s: Failed to close filter file %d", __func__, pos.nFile);
...
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r2048729097)
If we can't fully commit to disk, we won't call `fclose` directly - we'll rely on the destructor's close.
We'd get a commitment failure message (though that could have been successful) - and maybe another message from the destructor.
Could we separate the two errors?
```C++
if (!file.Commit()) {
LogError("%s: Failed to commit filter file %d", __func__, pos.nFile);
return false;
}
if (file.fclose()) {
LogError("%s: Failed to close filter file %d", __func__, pos.nFile);
...
💬 l0rinc commented on pull request "util: explicitly close all AutoFiles that have been written":
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r2048754656)
nit: now that we're closing explicitly we probably don't need the scope, too (wouldn't we double-log in that case anyway?) Or if we keep it, could make sense to add the closing inside - unless it's deliberate, in which case we might want to explain it in the commit message
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r2048754656)
nit: now that we're closing explicitly we probably don't need the scope, too (wouldn't we double-log in that case anyway?) Or if we keep it, could make sense to add the closing inside - unless it's deliberate, in which case we might want to explain it in the commit message
💬 l0rinc commented on pull request "util: explicitly close all AutoFiles that have been written":
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r2048747378)
Why is this a `LogInfo`, shouldn't it be a `LogError`?
```suggestion
if (fileout.fclose()) {
LogError("%s: Failed to close filter file %d: %s", __func__, pos.nFile, SysErrorString(errno));
return 0;
}
```
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r2048747378)
Why is this a `LogInfo`, shouldn't it be a `LogError`?
```suggestion
if (fileout.fclose()) {
LogError("%s: Failed to close filter file %d: %s", __func__, pos.nFile, SysErrorString(errno));
return 0;
}
```
💬 l0rinc commented on pull request "util: explicitly close all AutoFiles that have been written":
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r2048769760)
why do we want the destructor to be a noop?
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r2048769760)
why do we want the destructor to be a noop?
💬 l0rinc commented on pull request "util: explicitly close all AutoFiles that have been written":
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r2048772120)
nit: this likely isn't strictly necessary, as long as it contains all the data (checked below), it should work correctly - but it's fine either way
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r2048772120)
nit: this likely isn't strictly necessary, as long as it contains all the data (checked below), it should work correctly - but it's fine either way
💬 l0rinc commented on pull request "util: explicitly close all AutoFiles that have been written":
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r2048791750)
```suggestion
LogError("Failed to close file: %s", SysErrorString(errno));
```
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r2048791750)
```suggestion
LogError("Failed to close file: %s", SysErrorString(errno));
```
💬 l0rinc commented on pull request "util: explicitly close all AutoFiles that have been written":
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r2048785183)
Can you separate the `AutoFile` changes to a new commit, explaining these changes separately from the changes that are calling close explicitly?
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r2048785183)
Can you separate the `AutoFile` changes to a new commit, explaining these changes separately from the changes that are calling close explicitly?
💬 hebasto commented on pull request "depends: bump boost to 1.87.0 and use new CMake buildsystem":
(https://github.com/bitcoin/bitcoin/pull/30434#issuecomment-2812702334)
> > @hebasto Want to try putting that behind a variable and upstreaming it? It'd be nice if we didn't have to carry that patch.
>
> Sure thing!
>
> Done in [boostorg/test#445](https://github.com/boostorg/test/pull/445).
[Here](https://github.com/hebasto/bitcoin/commits/250417-boost-cmake/) is an updated branch based on the upstream changes.
(https://github.com/bitcoin/bitcoin/pull/30434#issuecomment-2812702334)
> > @hebasto Want to try putting that behind a variable and upstreaming it? It'd be nice if we didn't have to carry that patch.
>
> Sure thing!
>
> Done in [boostorg/test#445](https://github.com/boostorg/test/pull/445).
[Here](https://github.com/hebasto/bitcoin/commits/250417-boost-cmake/) is an updated branch based on the upstream changes.
💬 ismaelsadeeq commented on issue "Test Package Accept":
(https://github.com/bitcoin/bitcoin/issues/32160#issuecomment-2812704410)
Given that we want `testmempoolaccept` to relax the requirement that the transaction should be topologically connected due to the reasons discussed above, should we consider extending `submitpackage` to do the same since as you mentioned in practice, some transactions already in the mempool can cause the submitted package to become topological, the submitted package may not appear connected the outset because it was deduplicated (but it is with some in-mempool transaction).
> This allows us to
...
(https://github.com/bitcoin/bitcoin/issues/32160#issuecomment-2812704410)
Given that we want `testmempoolaccept` to relax the requirement that the transaction should be topologically connected due to the reasons discussed above, should we consider extending `submitpackage` to do the same since as you mentioned in practice, some transactions already in the mempool can cause the submitted package to become topological, the submitted package may not appear connected the outset because it was deduplicated (but it is with some in-mempool transaction).
> This allows us to
...