💬 maflcko commented on pull request "refactor: Add consteval uint256("str") and ParseHex("str")":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1686109312)
Maybe `BytesFromHex`?
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1686109312)
Maybe `BytesFromHex`?
💬 maflcko commented on pull request "refactor: Add consteval uint256("str") and ParseHex("str")":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1686115679)
nit: snake_case for new code: `write_it`, `str_it`, according to the dev notes.
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1686115679)
nit: snake_case for new code: `write_it`, `str_it`, according to the dev notes.
💬 maflcko commented on pull request "logging: Replace LogError and LogWarning with LogAlert":
(https://github.com/bitcoin/bitcoin/pull/30364#discussion_r1686133882)
> I think this should probably be `LogDebug` rather than `Alert` messages?
It may be good to check that at least one warning is emitted to the user. Otherwise, it may be harder for them to spot the config error at all? (However, if non-logging related changes are made, I wonder if such changes can be done in separate (preparatory) pull requests, similar to https://github.com/bitcoin/bitcoin/pull/30064, as they require in-depth low-level net knowledge?)
(https://github.com/bitcoin/bitcoin/pull/30364#discussion_r1686133882)
> I think this should probably be `LogDebug` rather than `Alert` messages?
It may be good to check that at least one warning is emitted to the user. Otherwise, it may be harder for them to spot the config error at all? (However, if non-logging related changes are made, I wonder if such changes can be done in separate (preparatory) pull requests, similar to https://github.com/bitcoin/bitcoin/pull/30064, as they require in-depth low-level net knowledge?)
💬 fjahr commented on pull request "validation: assumeutxo params mainnet":
(https://github.com/bitcoin/bitcoin/pull/28553#issuecomment-2242330336)
tACK f193b0fd853cb9e2f7cb4ed9dd6fb5d2662a04fe
I have re-tested the full assumeutxo flow using this snapshot: Downloaded the snapshot from the torrent link, verified the hash, successfully loaded the snapshot in a fresh node, verified that the snapshot chainstate was able to sync to the tip and the node was usable. Then waited until the background chainstate caught up to the snapshot and that was also successful. Restarted the node and verified the cleanup of the second chainstate. Also checke
...
(https://github.com/bitcoin/bitcoin/pull/28553#issuecomment-2242330336)
tACK f193b0fd853cb9e2f7cb4ed9dd6fb5d2662a04fe
I have re-tested the full assumeutxo flow using this snapshot: Downloaded the snapshot from the torrent link, verified the hash, successfully loaded the snapshot in a fresh node, verified that the snapshot chainstate was able to sync to the tip and the node was usable. Then waited until the background chainstate caught up to the snapshot and that was also successful. Restarted the node and verified the cleanup of the second chainstate. Also checke
...
💬 maflcko commented on pull request "rpc: doc: use "output script" terminology consistently in "asm"/"hex" results":
(https://github.com/bitcoin/bitcoin/pull/30408#discussion_r1686138362)
> `witness program` or `output script` seems like it might be more accurate than `witness output script`.
No. A witness program is different from the witness output script. The witness output script consists of both, the "version byte" and the "witness program". You can refer to BIP 141 for more details: https://en.bitcoin.it/wiki/BIP_0141#Witness_program
(https://github.com/bitcoin/bitcoin/pull/30408#discussion_r1686138362)
> `witness program` or `output script` seems like it might be more accurate than `witness output script`.
No. A witness program is different from the witness output script. The witness output script consists of both, the "version byte" and the "witness program". You can refer to BIP 141 for more details: https://en.bitcoin.it/wiki/BIP_0141#Witness_program
💬 maflcko commented on pull request "fuzz: reduce keypool size in `scriptpubkeyman` target":
(https://github.com/bitcoin/bitcoin/pull/30494#issuecomment-2242360509)
review ACK dcb4ec944984961e3b40452425a219e15e6ab508
This implements my suggestion from https://github.com/bitcoin/bitcoin/issues/30476#issue-2415683883, but I haven't benchmarked whether it works.
(https://github.com/bitcoin/bitcoin/pull/30494#issuecomment-2242360509)
review ACK dcb4ec944984961e3b40452425a219e15e6ab508
This implements my suggestion from https://github.com/bitcoin/bitcoin/issues/30476#issue-2415683883, but I haven't benchmarked whether it works.
💬 maflcko commented on pull request "qa: Functional test improvements":
(https://github.com/bitcoin/bitcoin/pull/30463#issuecomment-2242372890)
Discussions are resolved and this seems rfm with two reviews?
(https://github.com/bitcoin/bitcoin/pull/30463#issuecomment-2242372890)
Discussions are resolved and this seems rfm with two reviews?
💬 t-bast commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#issuecomment-2242378141)
> Stealing an ephemeral anchor output can only be done in certain conditions, at specific ranges of values. It is not always profitable due to overheads, and you can reduce those overheads by doing complex things.
I don't understand this hand-wavy comment, can you detail? I don't see any complexity here. Whenever a miner would include a transaction spending an ephemeral output in a block:
- if that ephemeral output is economical, the miner replaces the spending transaction by one that send
...
(https://github.com/bitcoin/bitcoin/pull/30239#issuecomment-2242378141)
> Stealing an ephemeral anchor output can only be done in certain conditions, at specific ranges of values. It is not always profitable due to overheads, and you can reduce those overheads by doing complex things.
I don't understand this hand-wavy comment, can you detail? I don't see any complexity here. Whenever a miner would include a transaction spending an ephemeral output in a block:
- if that ephemeral output is economical, the miner replaces the spending transaction by one that send
...
✅ Sjors closed a pull request: "Introduce waitFeesChanged() mining interface"
(https://github.com/bitcoin/bitcoin/pull/30443)
(https://github.com/bitcoin/bitcoin/pull/30443)
💬 Sjors commented on pull request "Introduce waitFeesChanged() mining interface":
(https://github.com/bitcoin/bitcoin/pull/30443#issuecomment-2242382625)
I'm moving this to https://github.com/Sjors/bitcoin/pull/52.
Since it's astronomically unlikely multiprocess will be in the upcoming v28.0 release, we have at least half a year to flesh out this interface. Hopefully cluster mempool will be further along by then, since that will inform the design.
(https://github.com/bitcoin/bitcoin/pull/30443#issuecomment-2242382625)
I'm moving this to https://github.com/Sjors/bitcoin/pull/52.
Since it's astronomically unlikely multiprocess will be in the upcoming v28.0 release, we have at least half a year to flesh out this interface. Hopefully cluster mempool will be further along by then, since that will inform the design.
💬 maflcko commented on issue "Fuzzing related configuration/build options can be improved":
(https://github.com/bitcoin/bitcoin/issues/30318#issuecomment-2242390222)
> > > * `--enable-fuzz-binary=yes|no|exclusive`, but renaming it would break every fuzzing script. Maybe the transition to CMake is a good time to improve this since scripts would have to be adjusted anyway.
> >
> >
> > Seems fine to do, if `-DBUILD_FUZZ_BINARY=ON/OFF/EXCL...` is easy to implement.
>
> It's not a problem at all.
On thing that would be confusing, is that `EXCL...` (or whatever the third state is) would have to also enable `-DABORT_ON_FAILED_ASSUME`. At this point, I qu
...
(https://github.com/bitcoin/bitcoin/issues/30318#issuecomment-2242390222)
> > > * `--enable-fuzz-binary=yes|no|exclusive`, but renaming it would break every fuzzing script. Maybe the transition to CMake is a good time to improve this since scripts would have to be adjusted anyway.
> >
> >
> > Seems fine to do, if `-DBUILD_FUZZ_BINARY=ON/OFF/EXCL...` is easy to implement.
>
> It's not a problem at all.
On thing that would be confusing, is that `EXCL...` (or whatever the third state is) would have to also enable `-DABORT_ON_FAILED_ASSUME`. At this point, I qu
...
💬 maflcko commented on pull request "refactor: Implement missing error checking for ArgsManager flags":
(https://github.com/bitcoin/bitcoin/pull/16545#issuecomment-2242444648)
> But fortunately I do not think we need to nitpick and argue about behavior the `Get*Arg` functions because **if any developer does not like the behavior of the `Get*Arg` functions they can call the `GetSetting` function instead** which gives access to the underlying, validated `UniValue` setting and allows any developers to implement any behavior they would like to implement to parse their settings using the UniValue API.
I looked at the feedback so far, and I wonder if it could make sense
...
(https://github.com/bitcoin/bitcoin/pull/16545#issuecomment-2242444648)
> But fortunately I do not think we need to nitpick and argue about behavior the `Get*Arg` functions because **if any developer does not like the behavior of the `Get*Arg` functions they can call the `GetSetting` function instead** which gives access to the underlying, validated `UniValue` setting and allows any developers to implement any behavior they would like to implement to parse their settings using the UniValue API.
I looked at the feedback so far, and I wonder if it could make sense
...
💬 maflcko commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#issuecomment-2242466297)
Checked that the code didn't change in the last push, only the commit message, which looked fine on a glance.
If you re-touch you can change "and scan past" to "and may scan past", or "and possibly scan past". Otherwise, it seems to imply this bug was actually hit.
Also, you can adjust the pull request description with a motivation and fix. Otherwise, it is just two random links, and reviewers will have a hard time seeing the motivation and gist of the changes.
(https://github.com/bitcoin/bitcoin/pull/30436#issuecomment-2242466297)
Checked that the code didn't change in the last push, only the commit message, which looked fine on a glance.
If you re-touch you can change "and scan past" to "and may scan past", or "and possibly scan past". Otherwise, it seems to imply this bug was actually hit.
Also, you can adjust the pull request description with a motivation and fix. Otherwise, it is just two random links, and reviewers will have a hard time seeing the motivation and gist of the changes.
💬 maflcko commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#issuecomment-2242466411)
re-ACK 788fe9cc9ab979c5e14f544ae05dc46436892b81 🔃
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: re-ACK 788fe9cc9ab979c5e
...
(https://github.com/bitcoin/bitcoin/pull/30436#issuecomment-2242466411)
re-ACK 788fe9cc9ab979c5e14f544ae05dc46436892b81 🔃
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: re-ACK 788fe9cc9ab979c5e
...
💬 maflcko commented on pull request "Stratum v2 connman":
(https://github.com/bitcoin/bitcoin/pull/30332#issuecomment-2242491437)
> reduce CI load
Some more machines were added a few weeks back to deal with increased overall activity, so I think the load should be handleable now, unless someone pushes to 10 different pull requests every 20 minutes in a loop.
Looks like you fixed it in the meantime, but if CI load comes up again, you may create an issue so that it can be fixed.
(https://github.com/bitcoin/bitcoin/pull/30332#issuecomment-2242491437)
> reduce CI load
Some more machines were added a few weeks back to deal with increased overall activity, so I think the load should be handleable now, unless someone pushes to 10 different pull requests every 20 minutes in a loop.
Looks like you fixed it in the meantime, but if CI load comes up again, you may create an issue so that it can be fixed.
💬 fanquake commented on pull request "depends: build zeromq with CMake":
(https://github.com/bitcoin/bitcoin/pull/29723#issuecomment-2242543346)
> CMake compiles 7 fewer source files compared to Autotools. It skips::
That's expected. We aren't opting in to either of these features.
(https://github.com/bitcoin/bitcoin/pull/29723#issuecomment-2242543346)
> CMake compiles 7 fewer source files compared to Autotools. It skips::
That's expected. We aren't opting in to either of these features.
💬 josibake commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#issuecomment-2242545417)
Update https://github.com/bitcoin/bitcoin/commit/b8b3a9f18670ec7bf246a57950cdae7e034a264d -> https://github.com/bitcoin/bitcoin/commit/56267cf84281789186035f09898cce3d749748ec ([apply-taptweak-method-04](https://github.com/josibake/bitcoin/tree/apply-taptweak-method-04) -> [apply-taptweak-method-05](https://github.com/josibake/bitcoin/tree/apply-taptweak-method-05) ([compare](https://github.com/josibake/bitcoin/compare/apply-taptweak-method-04..josibake:apply-taptweak-method-05))
Realised I u
...
(https://github.com/bitcoin/bitcoin/pull/30051#issuecomment-2242545417)
Update https://github.com/bitcoin/bitcoin/commit/b8b3a9f18670ec7bf246a57950cdae7e034a264d -> https://github.com/bitcoin/bitcoin/commit/56267cf84281789186035f09898cce3d749748ec ([apply-taptweak-method-04](https://github.com/josibake/bitcoin/tree/apply-taptweak-method-04) -> [apply-taptweak-method-05](https://github.com/josibake/bitcoin/tree/apply-taptweak-method-05) ([compare](https://github.com/josibake/bitcoin/compare/apply-taptweak-method-04..josibake:apply-taptweak-method-05))
Realised I u
...
💬 josibake commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1686283547)
Updated.
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1686283547)
Updated.
💬 Sjors commented on pull request "Have createNewBlock() return a BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/30440#discussion_r1686287447)
The only way this _should_ fail is `coinbase_script` is invalid (e.g. too large). But that currently results in a `throw std::runtime_error` inside `BlockAssembler::CreateNewBlock`.
Changed the three lines to `CHECK_NONFATAL`.
(https://github.com/bitcoin/bitcoin/pull/30440#discussion_r1686287447)
The only way this _should_ fail is `coinbase_script` is invalid (e.g. too large). But that currently results in a `throw std::runtime_error` inside `BlockAssembler::CreateNewBlock`.
Changed the three lines to `CHECK_NONFATAL`.
💬 Sjors commented on pull request "Have createNewBlock() return a BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/30440#discussion_r1686288232)
Done
(https://github.com/bitcoin/bitcoin/pull/30440#discussion_r1686288232)
Done