Bitcoin Core Github
44 subscribers
121K links
Download Telegram
💬 maflcko commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#issuecomment-3088867703)
review ACK 96da68a38fa295d2414685739c41b8626e198d27 🚋

<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: review ACK 96da68a38fa2
...
💬 maflcko commented on pull request "mempool: Avoid needless vtx iteration during IBD":
(https://github.com/bitcoin/bitcoin/pull/32827#discussion_r2215637564)
It is also updating the fee logic (`lastRollingFee`), so the comment isn't wrong. No strong opinion, though 😅
💬 maflcko commented on pull request "Cluster linearization: separate tests from tests-of-tests":
(https://github.com/bitcoin/bitcoin/pull/30605#discussion_r2215643371)
Could also use `mask|=non_empty;` (Maybe the `+` was a typo and meant `|`?)
💬 ismaelsadeeq commented on pull request "mempool: Avoid needless vtx iteration during IBD":
(https://github.com/bitcoin/bitcoin/pull/32827#discussion_r2215648241)
Okay, I see that :)
Yeah the comment matches that update, I guess that ~0 does not apply any more, can be resolved.
👍 ismaelsadeeq approved a pull request: "mempool: Avoid needless vtx iteration during IBD"
(https://github.com/bitcoin/bitcoin/pull/32827#pullrequestreview-3032959527)
Code review ACK 54f9cb85c4be2c30be0eb89f29b76a4cbf6f1c50
👍 stickies-v approved a pull request: "test: Do not pass tests on unhandled exceptions"
(https://github.com/bitcoin/bitcoin/pull/33001#pullrequestreview-3032970350)
re-ACK faa3e684118bffa7a98cf76eeeb59243219df900
💬 marcofleon commented on pull request "refactor: Convert GenTxid to `std::variant`":
(https://github.com/bitcoin/bitcoin/pull/32631#issuecomment-3088912601)
Follow ups to these last few comments can be found here: https://github.com/bitcoin/bitcoin/pull/33005
💬 rkrux commented on pull request "Have createwalletdescriptor auto-detect an unused(KEY)":
(https://github.com/bitcoin/bitcoin/pull/32861#issuecomment-3088939545)
Thanks, I recall reading this comment earlier but forgot about it later. If I am not missing anything, I don't suppose this point is orthogonal to this PR?

The linked comment also states that any RPC using the unused descriptor should delete it immediately afterwords.
> add logic to importdescriptors and generatewalletdescriptor to delete any unused(KEY) descriptor as soon as the KEY which it references is used by an new descriptor (taking into account both public and private parts of the k
...
💬 willcl-ark commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#issuecomment-3088988915)
Pushed c126475ed7a17ec9030066056e31846c7124dcf3 with a CI run on master branch at https://github.com/testing-cirrus-runners/bitcoin2/actions/runs/16368410249
💬 willcl-ark commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2215709407)
switched to `job-id` in 9458cd0e66d
💬 willcl-ark commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2215710431)
This now uses `git rev-parse HEAD:depends` in both 868ddf55718 and 1b689cf7811
💬 willcl-ark commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2215711488)
comment removed in 9458cd0e66d
💬 willcl-ark commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2215715420)
All references to `master` have been switched to `github.event.repository.default_branch` where appropriate. Removed the `ref_name` from the save/restore keys, as it is indeed unnecessary.
💬 willcl-ark commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2215717870)
9458cd0e66d now uses a depends sources cache per job_id to avoid this pitfall (thanks!):

```
key: depends-sources-${{ inputs.job-id }}-${{ env.DEPENDS_HASH }}
```
💬 willcl-ark commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2215719446)
Moved this code into the main `ci.yml` file, so that the cache actions are *only* performing caching (rather than being responsible for setting env vars), both in 868ddf55718 and 1b689cf7811
💬 willcl-ark commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2215719957)
Comment added in db78efac768
💬 willcl-ark commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2215720797)
Registry env vars updated in 7272f56a755
💬 willcl-ark commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#issuecomment-3089009002)
Thanks for the review @maflcko, I hope I addressed all your current review comments.
💬 ismaelsadeeq commented on pull request "Cluster linearization: separate tests from tests-of-tests":
(https://github.com/bitcoin/bitcoin/pull/30605#discussion_r2215722531)
Maybe not because comment above explicitly mentioned add, but that can be modified to match |.

see https://github.com/ismaelsadeeq/bitcoin/commit/95ea4e737612c094dfcb6414f8005c79c6a34551
🤔 maflcko reviewed a pull request: "[IBD] multi-byte block obfuscation"
(https://github.com/bitcoin/bitcoin/pull/31144#pullrequestreview-3033068660)
Not sure the code is correct (see comment), but I've looked at it:

review ACK 248b6a27c351690d3596711cc36b8102977adeab 🎻

<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+
...