💬 BenWestgate commented on pull request "Drop -dbcache limit":
(https://github.com/bitcoin/bitcoin/pull/28358#issuecomment-2347927495)
> I ran benchmarks to evaluate the impact of removing the limit on IBD performance. ... I synced up 3 times to block height 600,000...
Thanks for this benchmark. The current block height is 861,090 and the size of the utxo set has more than doubled since 600,000 so this is why you do not see performance improvements from dbcache > 10 GB.
> Based on this data I think removing the limit may not be substantiated...
Try syncing to 860,000 there should be performance benefit up to 30 GiB dbc
...
(https://github.com/bitcoin/bitcoin/pull/28358#issuecomment-2347927495)
> I ran benchmarks to evaluate the impact of removing the limit on IBD performance. ... I synced up 3 times to block height 600,000...
Thanks for this benchmark. The current block height is 861,090 and the size of the utxo set has more than doubled since 600,000 so this is why you do not see performance improvements from dbcache > 10 GB.
> Based on this data I think removing the limit may not be substantiated...
Try syncing to 860,000 there should be performance benefit up to 30 GiB dbc
...
💬 davidgumberg commented on pull request "streams: cache file position within AutoFile":
(https://github.com/bitcoin/bitcoin/pull/30884#discussion_r1758096576)
Only commenting because this confused me:
This makes sense since the third possibility here is that `origin == SEEK_END`, in which case the only way for `fseek` to have returned zero is if `offset` is a negative number offset from the end of the file, and `m_position = length_of_file + negative_offset`
But, in order to get the length of the fil you need to `fseek(m_file, 0, SEEK_END)` and `ftell(m_file)`.
So, just `ftell` here is the best way to get the offset.
(https://github.com/bitcoin/bitcoin/pull/30884#discussion_r1758096576)
Only commenting because this confused me:
This makes sense since the third possibility here is that `origin == SEEK_END`, in which case the only way for `fseek` to have returned zero is if `offset` is a negative number offset from the end of the file, and `m_position = length_of_file + negative_offset`
But, in order to get the length of the fil you need to `fseek(m_file, 0, SEEK_END)` and `ftell(m_file)`.
So, just `ftell` here is the best way to get the offset.
🤔 BenWestgate reviewed a pull request: "Drop -dbcache limit"
(https://github.com/bitcoin/bitcoin/pull/28358#pullrequestreview-2302029807)
crACK bb3b980dfd96d9a89e6b04aef2859ce0555c6807.
Setting `dbcache` below available RAM performs better and swaps less than setting it below total RAM, which may even crash the machine or OOM kill other applications.
I feel the tool tip and argument texts should say "available RAM" but it's not a blocker for me.
(https://github.com/bitcoin/bitcoin/pull/28358#pullrequestreview-2302029807)
crACK bb3b980dfd96d9a89e6b04aef2859ce0555c6807.
Setting `dbcache` below available RAM performs better and swaps less than setting it below total RAM, which may even crash the machine or OOM kill other applications.
I feel the tool tip and argument texts should say "available RAM" but it's not a blocker for me.
💬 BenWestgate commented on pull request "Drop -dbcache limit":
(https://github.com/bitcoin/bitcoin/pull/28358#discussion_r1758106872)
Nit: "Make sure you have enough available RAM." seems more correct.
(https://github.com/bitcoin/bitcoin/pull/28358#discussion_r1758106872)
Nit: "Make sure you have enough available RAM." seems more correct.
💬 BenWestgate commented on pull request "Drop -dbcache limit":
(https://github.com/bitcoin/bitcoin/pull/28358#discussion_r1758098327)
nit: This should say "RAM available" rather than "RAM".
On a 64 GB system without swap: it will crash if you set `dbcache=32000` but only have only have 20 GB available.
On a 64 GB system with enough swap: it will perform MUCH worse and prematurely wear out the flash storage, if you set `dbcache=32000` but only have only have 20 GB available RAM.
(I've destroyed new USB sticks overnight, putting a swapfile on them and doing IBD with `dbcache` set a little too high.)
The only reason
...
(https://github.com/bitcoin/bitcoin/pull/28358#discussion_r1758098327)
nit: This should say "RAM available" rather than "RAM".
On a 64 GB system without swap: it will crash if you set `dbcache=32000` but only have only have 20 GB available.
On a 64 GB system with enough swap: it will perform MUCH worse and prematurely wear out the flash storage, if you set `dbcache=32000` but only have only have 20 GB available RAM.
(I've destroyed new USB sticks overnight, putting a swapfile on them and doing IBD with `dbcache` set a little too high.)
The only reason
...
💬 davidgumberg commented on pull request "streams: cache file position within AutoFile":
(https://github.com/bitcoin/bitcoin/pull/30884#discussion_r1758143719)
Outside of the scope of this PR: I feel like this should probably handle a failed `fread` explicitly, even though the subspan call below means XOR won't do anything bad here, and when this returns to `AutoFile::read()` or `BufferedFile::fill()` (it's only two callers) the error gets handled. It seems kind of footgun-like to let this potentially dangerous file pointer get passed around and I'm not sure it's worth `BufferedFile` being able to print it's own name when throwing an exception.
(https://github.com/bitcoin/bitcoin/pull/30884#discussion_r1758143719)
Outside of the scope of this PR: I feel like this should probably handle a failed `fread` explicitly, even though the subspan call below means XOR won't do anything bad here, and when this returns to `AutoFile::read()` or `BufferedFile::fill()` (it's only two callers) the error gets handled. It seems kind of footgun-like to let this potentially dangerous file pointer get passed around and I'm not sure it's worth `BufferedFile` being able to print it's own name when throwing an exception.
💬 davidgumberg commented on pull request "streams: cache file position within AutoFile":
(https://github.com/bitcoin/bitcoin/pull/30884#discussion_r1758185734)
clang-tidy seems [upset](https://github.com/bitcoin/bitcoin/pull/30884/checks?check_run_id=30079004954) about setting `m_position{0}` here instead of using C++11 "Default member initialization" in the class declaration (https://clang.llvm.org/extra/clang-tidy/checks/modernize/use-default-member-init.html)
(https://github.com/bitcoin/bitcoin/pull/30884#discussion_r1758185734)
clang-tidy seems [upset](https://github.com/bitcoin/bitcoin/pull/30884/checks?check_run_id=30079004954) about setting `m_position{0}` here instead of using C++11 "Default member initialization" in the class declaration (https://clang.llvm.org/extra/clang-tidy/checks/modernize/use-default-member-init.html)
👍 jarolrod approved a pull request: "gui: fix crash when closing wallet"
(https://github.com/bitcoin-core/gui/pull/835#pullrequestreview-2302134082)
ACK a965f2bc07a3588f8c2b8d6a542961562e3f5d0e
This accurately diagnoses and fixes the referenced issue. I've thoroughly tested this on Linux, macOS, and Windows; and both confirm the existence of the bug and that this fixes it. I've also sanity tested other related actions in the GUI in different scenarios, and also sanity tested other non-wallet actions in the GUI for crashes.
Thanks for the quick fix :)
(https://github.com/bitcoin-core/gui/pull/835#pullrequestreview-2302134082)
ACK a965f2bc07a3588f8c2b8d6a542961562e3f5d0e
This accurately diagnoses and fixes the referenced issue. I've thoroughly tested this on Linux, macOS, and Windows; and both confirm the existence of the bug and that this fixes it. I've also sanity tested other related actions in the GUI in different scenarios, and also sanity tested other non-wallet actions in the GUI for crashes.
Thanks for the quick fix :)
💬 davidgumberg commented on pull request "streams: cache file position within AutoFile":
(https://github.com/bitcoin/bitcoin/pull/30884#discussion_r1758190241)
nit: Handle ftell error
```suggestion
if (pos < 0) {
throw std::ios_base::failure("AutoFile::seek: ftell failed");
}
m_position = pos;
```
(https://github.com/bitcoin/bitcoin/pull/30884#discussion_r1758190241)
nit: Handle ftell error
```suggestion
if (pos < 0) {
throw std::ios_base::failure("AutoFile::seek: ftell failed");
}
m_position = pos;
```
💬 ariard commented on pull request "Halt processing of unrequested transactions v2":
(https://github.com/bitcoin/bitcoin/pull/30572#issuecomment-2348023783)
@ajtowns
> If this is just something a peer can opt-in to, I don't think it mitigates any DoS behaviours: spammers/attackers will just use the old version.
Yes, indeed this is a quite obvious point about a node service bit. It was raised during the mempoolfullrbf discussion years ago (see #25600), that effectively node service bit network mechanism cannot be effectively checked for, without actually connecting to and triggering the logic. About the present case, this observation has been
...
(https://github.com/bitcoin/bitcoin/pull/30572#issuecomment-2348023783)
@ajtowns
> If this is just something a peer can opt-in to, I don't think it mitigates any DoS behaviours: spammers/attackers will just use the old version.
Yes, indeed this is a quite obvious point about a node service bit. It was raised during the mempoolfullrbf discussion years ago (see #25600), that effectively node service bit network mechanism cannot be effectively checked for, without actually connecting to and triggering the logic. About the present case, this observation has been
...
💬 ajtowns commented on pull request "signet: fixing mining for OP_TRUE challenge":
(https://github.com/bitcoin/bitcoin/pull/29032#discussion_r1756454064)
python assert doesn't need brackets
This could have (rare, but not cryptographically-rare) false positives if the signet commitment value appears within the segwit commitment; could check it correctly with:
```python
from test_framework.script import CScript
def get_signet_commitment(segwit_commitment):
for el in CScript.fromhex(segwit_commitment):
if isinstance(el, bytes) and el[0:4].hex() == SIGNET_COMMITMENT:
return el[4:].hex()
return None
#assert
...
(https://github.com/bitcoin/bitcoin/pull/29032#discussion_r1756454064)
python assert doesn't need brackets
This could have (rare, but not cryptographically-rare) false positives if the signet commitment value appears within the segwit commitment; could check it correctly with:
```python
from test_framework.script import CScript
def get_signet_commitment(segwit_commitment):
for el in CScript.fromhex(segwit_commitment):
if isinstance(el, bytes) and el[0:4].hex() == SIGNET_COMMITMENT:
return el[4:].hex()
return None
#assert
...
💬 ajtowns commented on pull request "signet: fixing mining for OP_TRUE challenge":
(https://github.com/bitcoin/bitcoin/pull/29032#discussion_r1758151930)
This comment should probably be in the "advanced usage" section at the bottom? I think the PSBT is still technically necessary there, it's just that you'd go `getblocktemplate | genpsbt | solvepsbt | submitblock` skipping the `walletprocesspsbt | jq -r .psbt` step as there's nothing to sign.
Hmm, except that doesn't actually work, since only `generate` has been updated to work with trivial challenges. I think `solvepsbt` should be also, which can be done by:
```diff
-def get_solution_from
...
(https://github.com/bitcoin/bitcoin/pull/29032#discussion_r1758151930)
This comment should probably be in the "advanced usage" section at the bottom? I think the PSBT is still technically necessary there, it's just that you'd go `getblocktemplate | genpsbt | solvepsbt | submitblock` skipping the `walletprocesspsbt | jq -r .psbt` step as there's nothing to sign.
Hmm, except that doesn't actually work, since only `generate` has been updated to work with trivial challenges. I think `solvepsbt` should be also, which can be done by:
```diff
-def get_solution_from
...
💬 ajtowns commented on pull request "signet: fixing mining for OP_TRUE challenge":
(https://github.com/bitcoin/bitcoin/pull/29032#discussion_r1758225739)
Could consider checking manual mining as well:
```python
# generate block with signet miner tool
def mine_block_manual(self, node):
assert_equal(node.getblockcount(), 1)
base_dir = self.config["environment"]["SRCDIR"]
signet_miner_path = os.path.join(base_dir, "contrib", "signet", "miner")
base_cmd = [
sys.executable,
signet_miner_path,
f'--cli={node.cli.binary} -datadir={node.cli.datadir}',
...
(https://github.com/bitcoin/bitcoin/pull/29032#discussion_r1758225739)
Could consider checking manual mining as well:
```python
# generate block with signet miner tool
def mine_block_manual(self, node):
assert_equal(node.getblockcount(), 1)
base_dir = self.config["environment"]["SRCDIR"]
signet_miner_path = os.path.join(base_dir, "contrib", "signet", "miner")
base_cmd = [
sys.executable,
signet_miner_path,
f'--cli={node.cli.binary} -datadir={node.cli.datadir}',
...
💬 laanwj commented on pull request "streams: cache file position within AutoFile":
(https://github.com/bitcoin/bitcoin/pull/30884#discussion_r1758234047)
Handling i/o errors (eg partial reads/writes can be considered fatal for disk i/o) is always important, but yes even more-so now that the file pointer is cached and might get out of sync with the world.
(https://github.com/bitcoin/bitcoin/pull/30884#discussion_r1758234047)
Handling i/o errors (eg partial reads/writes can be considered fatal for disk i/o) is always important, but yes even more-so now that the file pointer is cached and might get out of sync with the world.
⚠️ maflcko opened an issue: "Intermittent failure in tool_wallet.py in self.assert_tool_output('', '-wallet=salvage', 'salvage') : assert_equal(p.poll(), 0) ; AssertionError: not(3221226505 == 0)"
(https://github.com/bitcoin/bitcoin/issues/30894)
https://github.com/maflcko/bitcoin-core-with-ci/actions/runs/10833872949/job/30079686125#step:12:1068
```
test 2024-09-12T23:30:54.661000Z TestFramework (INFO): Check salvage
...
node0 2024-09-12T23:30:55.537324Z [shutoff] [D:\a\bitcoin-core-with-ci\bitcoin-core-with-ci\src\wallet\bdb.cpp:608] [Flush] [walletdb] BerkeleyEnvironment::Flush: [D:\a\_temp\test_runner_₿_🏃_20240912_231657\tool_wallet_222\node0\regtest\wallets\salvage] Flush(false) database not started
node0 2024-09
...
(https://github.com/bitcoin/bitcoin/issues/30894)
https://github.com/maflcko/bitcoin-core-with-ci/actions/runs/10833872949/job/30079686125#step:12:1068
```
test 2024-09-12T23:30:54.661000Z TestFramework (INFO): Check salvage
...
node0 2024-09-12T23:30:55.537324Z [shutoff] [D:\a\bitcoin-core-with-ci\bitcoin-core-with-ci\src\wallet\bdb.cpp:608] [Flush] [walletdb] BerkeleyEnvironment::Flush: [D:\a\_temp\test_runner_₿_🏃_20240912_231657\tool_wallet_222\node0\regtest\wallets\salvage] Flush(false) database not started
node0 2024-09
...
💬 jaybny commented on issue "GUI event loop should be block free":
(https://github.com/bitcoin/bitcoin/issues/17145#issuecomment-2348122731)
qt has a tool that will build on android with qmake. not sure how much work
this would be, but it's doable. ffiw
On Thu, Sep 12, 2024, 4:57 AM maflcko ***@***.***> wrote:
> Ok, and Android support was blocked on cmake or qt6, or was it both?
> (Sorry for using this thread, but I couldn't find the tracking issue).
>
> —
> Reply to this email directly, view it on GitHub
> <https://github.com/bitcoin/bitcoin/issues/17145#issuecomment-2346089167>,
> or unsubscribe
> <https://github.com
...
(https://github.com/bitcoin/bitcoin/issues/17145#issuecomment-2348122731)
qt has a tool that will build on android with qmake. not sure how much work
this would be, but it's doable. ffiw
On Thu, Sep 12, 2024, 4:57 AM maflcko ***@***.***> wrote:
> Ok, and Android support was blocked on cmake or qt6, or was it both?
> (Sorry for using this thread, but I couldn't find the tracking issue).
>
> —
> Reply to this email directly, view it on GitHub
> <https://github.com/bitcoin/bitcoin/issues/17145#issuecomment-2346089167>,
> or unsubscribe
> <https://github.com
...
💬 davidgumberg commented on pull request "streams: cache file position within AutoFile":
(https://github.com/bitcoin/bitcoin/pull/30884#discussion_r1758262463)
Out of scope but I found while reviewing: Is there any reason why `AutoFile::ignore()` couldn't be removed and `AutoFile::seek(nSize, SEEK_CURR)` used instead?
(https://github.com/bitcoin/bitcoin/pull/30884#discussion_r1758262463)
Out of scope but I found while reviewing: Is there any reason why `AutoFile::ignore()` couldn't be removed and `AutoFile::seek(nSize, SEEK_CURR)` used instead?
💬 naumenkogs commented on pull request "fuzz: Test headers pre-sync through p2p":
(https://github.com/bitcoin/bitcoin/pull/30661#issuecomment-2348253473)
ACK a97f43d63a6e835bae20b0bc5d536df98f55d8a0
(https://github.com/bitcoin/bitcoin/pull/30661#issuecomment-2348253473)
ACK a97f43d63a6e835bae20b0bc5d536df98f55d8a0
💬 maflcko commented on issue "Increasing self-hosted runner raw performance":
(https://github.com/bitcoin/bitcoin/issues/30852#issuecomment-2348255358)
The thing is that workers are "shuffled" by type, so one run may happen one one machine and another run of the same task may happen on another machine. It is of course possible to make each task its own type, but that seems a bit too much maintenance overhead when tasks are added or removed in one branch, but not all. As you mentioned earlier, it is of course also possible to set up a remote ccache. However, I think this is similarly going to be more maintenance overhead and extra moving parts,
...
(https://github.com/bitcoin/bitcoin/issues/30852#issuecomment-2348255358)
The thing is that workers are "shuffled" by type, so one run may happen one one machine and another run of the same task may happen on another machine. It is of course possible to make each task its own type, but that seems a bit too much maintenance overhead when tasks are added or removed in one branch, but not all. As you mentioned earlier, it is of course also possible to set up a remote ccache. However, I think this is similarly going to be more maintenance overhead and extra moving parts,
...
💬 naumenkogs commented on pull request "refactor: move m_is_inbound out of CNodeState":
(https://github.com/bitcoin/bitcoin/pull/30233#discussion_r1758356166)
should we assert the result of this code instead of dropping it on the floor?
(https://github.com/bitcoin/bitcoin/pull/30233#discussion_r1758356166)
should we assert the result of this code instead of dropping it on the floor?