💬 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?
👍 naumenkogs approved a pull request: "refactor: move m_is_inbound out of CNodeState"
(https://github.com/bitcoin/bitcoin/pull/30233#pullrequestreview-2302367391)
ACK 07f4cebe5780f1039541d989e64b70eccc5b4eb5
(https://github.com/bitcoin/bitcoin/pull/30233#pullrequestreview-2302367391)
ACK 07f4cebe5780f1039541d989e64b70eccc5b4eb5
👍 naumenkogs approved a pull request: "test: addrman: tried 3 times and never a success so `isTerrible=true`"
(https://github.com/bitcoin/bitcoin/pull/30445#pullrequestreview-2302379127)
ACK d6fabbe2d40b53f6fa35d6b0191f3d73999d6b41
(https://github.com/bitcoin/bitcoin/pull/30445#pullrequestreview-2302379127)
ACK d6fabbe2d40b53f6fa35d6b0191f3d73999d6b41
💬 petertodd commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#issuecomment-2348274066)
Re: dust UTXO creation, MARA has told me directly that they do not enforce the dust limit at all, and IIUC F2Pool does not as well. I haven't however personally tested this.
On September 13, 2024 3:08:05 AM GMT+03:00, Suhas Daftuar ***@***.***> wrote:
>@sdaftuar commented on this pull request.
>
>
>
>> @@ -934,6 +935,13 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
> fSpendsCoinbase, nSigOpsCost, lock_points.value()));
> ws.m_vsize = ent
...
(https://github.com/bitcoin/bitcoin/pull/30239#issuecomment-2348274066)
Re: dust UTXO creation, MARA has told me directly that they do not enforce the dust limit at all, and IIUC F2Pool does not as well. I haven't however personally tested this.
On September 13, 2024 3:08:05 AM GMT+03:00, Suhas Daftuar ***@***.***> wrote:
>@sdaftuar commented on this pull request.
>
>
>
>> @@ -934,6 +935,13 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
> fSpendsCoinbase, nSigOpsCost, lock_points.value()));
> ws.m_vsize = ent
...
💬 maflcko commented on issue "Nuke format linter":
(https://github.com/bitcoin/bitcoin/issues/30530#issuecomment-2348277045)
The next step is https://github.com/bitcoin/bitcoin/pull/30889
(https://github.com/bitcoin/bitcoin/issues/30530#issuecomment-2348277045)
The next step is https://github.com/bitcoin/bitcoin/pull/30889
💬 petertodd commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1758381743)
@sdaftuar
Re: dust UTXO creation, MARA has told me directly that they do not enforce the dust limit at all, and IIUC F2Pool does not as well. I haven't however personally tested this.
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1758381743)
@sdaftuar
Re: dust UTXO creation, MARA has told me directly that they do not enforce the dust limit at all, and IIUC F2Pool does not as well. I haven't however personally tested this.