💬 darosior commented on pull request "fuzz: a target for the block index database":
(https://github.com/bitcoin/bitcoin/pull/28209#discussion_r1284480443)
Trying to maximum the coverage in 100_000 runs for each target i managed to:
- achieve `2099` of coverage with the `ConsumeDeserializable`-based target using `-max_len=10000 -len_control=1 -mutate_depth=3` (in 305 seconds).
- achieve `2088` of coverage with the `ConsumeIntegral`-based target using -max_len=8000 -len_control=0 -use_value_profile=1 -mutate_depth=1` (in 252 seconds).
------
> I don't think you can use coverage as a metric
Oh, right.. I had overlooked this. However the di
...
(https://github.com/bitcoin/bitcoin/pull/28209#discussion_r1284480443)
Trying to maximum the coverage in 100_000 runs for each target i managed to:
- achieve `2099` of coverage with the `ConsumeDeserializable`-based target using `-max_len=10000 -len_control=1 -mutate_depth=3` (in 305 seconds).
- achieve `2088` of coverage with the `ConsumeIntegral`-based target using -max_len=8000 -len_control=0 -use_value_profile=1 -mutate_depth=1` (in 252 seconds).
------
> I don't think you can use coverage as a metric
Oh, right.. I had overlooked this. However the di
...
💬 MarcoFalke commented on issue "ci: Future of macOS and Windows MSVC CI tasks":
(https://github.com/bitcoin/bitcoin/issues/28098#issuecomment-1665708491)
> This will remove Cirrus from the loop entirely
Yes, that should work. The only difference seems to be that assigning labels is only possible manual (https://docs.github.com/en/actions/hosting-your-own-runners/managing-self-hosted-runners/using-labels-with-self-hosted-runners) and not from the command line that starts the runner.
But I guess in the short run it seems easier to stick to Cirrus for now, because the diff is a lot smaller (just replace `container:` in the yml with `persistent
...
(https://github.com/bitcoin/bitcoin/issues/28098#issuecomment-1665708491)
> This will remove Cirrus from the loop entirely
Yes, that should work. The only difference seems to be that assigning labels is only possible manual (https://docs.github.com/en/actions/hosting-your-own-runners/managing-self-hosted-runners/using-labels-with-self-hosted-runners) and not from the command line that starts the runner.
But I guess in the short run it seems easier to stick to Cirrus for now, because the diff is a lot smaller (just replace `container:` in the yml with `persistent
...
💬 MarcoFalke commented on pull request "fuzz: a target for the block index database":
(https://github.com/bitcoin/bitcoin/pull/28209#discussion_r1284506824)
> I'm starting to lean toward reverting to this version.
There's also the possibility to add back the early return to the `ConsumeIntegral` one. (I haven't looked at the fuzz target to see if early return makes more or less sense)
> I feel like introducing a crash wouldn't be interesting given the low coverage of the target.
yeah, I guess it is hard to find a meaningful crash. I'd pick a line of code that is usually reached the "last" by coverage or is deeply nested.
(https://github.com/bitcoin/bitcoin/pull/28209#discussion_r1284506824)
> I'm starting to lean toward reverting to this version.
There's also the possibility to add back the early return to the `ConsumeIntegral` one. (I haven't looked at the fuzz target to see if early return makes more or less sense)
> I feel like introducing a crash wouldn't be interesting given the low coverage of the target.
yeah, I guess it is hard to find a meaningful crash. I'd pick a line of code that is usually reached the "last" by coverage or is deeply nested.
💬 hebasto commented on pull request "ci: Run "macOS native x86_64" job on GitHub Actions":
(https://github.com/bitcoin/bitcoin/pull/28187#discussion_r1284516335)
[[Usage limits](https://docs.github.com/en/actions/learn-github-actions/usage-limits-billing-and-administration#usage-limits)](https://docs.github.com/en/actions/learn-github-actions/usage-limits-billing-and-administration#usage-limits):
> Each job in a workflow can run for up to 6 hours of execution time.
(https://github.com/bitcoin/bitcoin/pull/28187#discussion_r1284516335)
[[Usage limits](https://docs.github.com/en/actions/learn-github-actions/usage-limits-billing-and-administration#usage-limits)](https://docs.github.com/en/actions/learn-github-actions/usage-limits-billing-and-administration#usage-limits):
> Each job in a workflow can run for up to 6 hours of execution time.
💬 furszy commented on pull request "indexes: Stop using node internal types and locking cs_main, improve sync logic":
(https://github.com/bitcoin/bitcoin/pull/24230#discussion_r1284517464)
Yeah ok, that is also a possibility. Great.
Could the fix be decoupled into a standalone PR? Sounds like something that we could rapidly merge independently of this PR. It fixes a bug and also makes this work smaller.
(https://github.com/bitcoin/bitcoin/pull/24230#discussion_r1284517464)
Yeah ok, that is also a possibility. Great.
Could the fix be decoupled into a standalone PR? Sounds like something that we could rapidly merge independently of this PR. It fixes a bug and also makes this work smaller.
💬 MarcoFalke commented on pull request "ci: Run "macOS native x86_64" job on GitHub Actions":
(https://github.com/bitcoin/bitcoin/pull/28187#discussion_r1284518653)
May be better to reduce it to two hours to catch timeout issues earlier? But no strong opinion, just a nit.
(https://github.com/bitcoin/bitcoin/pull/28187#discussion_r1284518653)
May be better to reduce it to two hours to catch timeout issues earlier? But no strong opinion, just a nit.
💬 hebasto commented on pull request "ci: Run "macOS native x86_64" job on GitHub Actions":
(https://github.com/bitcoin/bitcoin/pull/28187#discussion_r1284522205)
Done.
(https://github.com/bitcoin/bitcoin/pull/28187#discussion_r1284522205)
Done.
💬 hebasto commented on pull request "ci: Run "macOS native x86_64" job on GitHub Actions":
(https://github.com/bitcoin/bitcoin/pull/28187#discussion_r1284522643)
Restored.
(https://github.com/bitcoin/bitcoin/pull/28187#discussion_r1284522643)
Restored.
💬 MarcoFalke commented on pull request "ci: Run "macOS native x86_64" job on GitHub Actions":
(https://github.com/bitcoin/bitcoin/pull/28187#issuecomment-1665737884)
re-ACK fe34609476b11cdd907c298f7300d424bf556e98 🐺
<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 fe34609476b11cdd907
...
(https://github.com/bitcoin/bitcoin/pull/28187#issuecomment-1665737884)
re-ACK fe34609476b11cdd907c298f7300d424bf556e98 🐺
<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 fe34609476b11cdd907
...
💬 furszy commented on pull request "Bugfix: net_processing: Restore "Already requested" error for FetchBlock":
(https://github.com/bitcoin/bitcoin/pull/28055#discussion_r1284531773)
In 2b67ea46:
Instead of adding the new function, could just delete the `RemoveBlockRequest()` line and directly call `BlockRequested()`. Failing with a `Already requested from this peer` if the call return false.
`BlockRequested()` only fails when the block is already in-flight for that peer (it does the same as your new `IsBlockRequestedFromPeer()` function).
(https://github.com/bitcoin/bitcoin/pull/28055#discussion_r1284531773)
In 2b67ea46:
Instead of adding the new function, could just delete the `RemoveBlockRequest()` line and directly call `BlockRequested()`. Failing with a `Already requested from this peer` if the call return false.
`BlockRequested()` only fails when the block is already in-flight for that peer (it does the same as your new `IsBlockRequestedFromPeer()` function).
🤔 glozow reviewed a pull request: "p2p: Drop m_recently_announced_invs bloom filter"
(https://github.com/bitcoin/bitcoin/pull/27675#pullrequestreview-1563072550)
ACK 1a118062fbc4ec8f645f4ec4298d869a869c3344
(https://github.com/bitcoin/bitcoin/pull/27675#pullrequestreview-1563072550)
ACK 1a118062fbc4ec8f645f4ec4298d869a869c3344
💬 glozow commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1284547085)
> Therefore checking than we have available UTXOs in mempool (or in-package) to spend could be done before to call to AncestorPackage ctor
I don't think this is true. We should sort the transactions before we look up UTXOs.
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1284547085)
> Therefore checking than we have available UTXOs in mempool (or in-package) to spend could be done before to call to AncestorPackage ctor
I don't think this is true. We should sort the transactions before we look up UTXOs.
💬 MarcoFalke commented on pull request "test: create wallet specific for test_locked_wallet case":
(https://github.com/bitcoin/bitcoin/pull/28139#discussion_r1284563239)
This fails locally:
```
node1 2023-08-02T03:08:04.791676Z [httpworker.2] [rpc/request.cpp:181] [parse] [rpc] ThreadRPCServer method=getnewaddress user=__cookie__
node1 2023-08-02T03:08:04.798619Z [httpworker.2] [wallet/scriptpubkeyman.h:254] [WalletLogPrintf] [locked_wallet] keypool reserve 3
node1 2023-08-02T03:08:04.807232Z [httpworker.2] [wallet/scriptpubkeyman.h:254] [WalletLogPrintf] [locked_wallet] keypool keep 3
node1 2023-08-02T03:08:04.883158Z [http] [httpserver.cpp:255] [
...
(https://github.com/bitcoin/bitcoin/pull/28139#discussion_r1284563239)
This fails locally:
```
node1 2023-08-02T03:08:04.791676Z [httpworker.2] [rpc/request.cpp:181] [parse] [rpc] ThreadRPCServer method=getnewaddress user=__cookie__
node1 2023-08-02T03:08:04.798619Z [httpworker.2] [wallet/scriptpubkeyman.h:254] [WalletLogPrintf] [locked_wallet] keypool reserve 3
node1 2023-08-02T03:08:04.807232Z [httpworker.2] [wallet/scriptpubkeyman.h:254] [WalletLogPrintf] [locked_wallet] keypool keep 3
node1 2023-08-02T03:08:04.883158Z [http] [httpserver.cpp:255] [
...
💬 MarcoFalke commented on pull request "test: create wallet specific for test_locked_wallet case":
(https://github.com/bitcoin/bitcoin/pull/28139#discussion_r1284568755)
https://drahtbot.space/temp_scratch/wallet_fundrawtransaction_251.tar.xz
(https://github.com/bitcoin/bitcoin/pull/28139#discussion_r1284568755)
https://drahtbot.space/temp_scratch/wallet_fundrawtransaction_251.tar.xz
💬 willcl-ark commented on pull request "net: disconnect inside AttemptToEvictConnection":
(https://github.com/bitcoin/bitcoin/pull/27912#issuecomment-1665802905)
Thanks for the review and patch @vasild, I will try to review it over the weekend and keep this moving forward.
I have in the mean time been considering a totally different approach to thread-safe deletion of nodes from `CConnman`, but it's somewhat more invasive (in terms of LOC)... I was experimenting with whether `m_nodes` would work better as a vector of `share_ptr`, meaning they could be added and removed from `m_nodes` (and `m_nodes_disconnected`) much more safely from the perspective o
...
(https://github.com/bitcoin/bitcoin/pull/27912#issuecomment-1665802905)
Thanks for the review and patch @vasild, I will try to review it over the weekend and keep this moving forward.
I have in the mean time been considering a totally different approach to thread-safe deletion of nodes from `CConnman`, but it's somewhat more invasive (in terms of LOC)... I was experimenting with whether `m_nodes` would work better as a vector of `share_ptr`, meaning they could be added and removed from `m_nodes` (and `m_nodes_disconnected`) much more safely from the perspective o
...
💬 ryanofsky commented on pull request "indexes: Stop using node internal types and locking cs_main, improve sync logic":
(https://github.com/bitcoin/bitcoin/pull/24230#discussion_r1284581060)
> Yeah ok, that is also a possibility. Great.
>
> Could the fix be decoupled into a standalone PR? Sounds like something that we could rapidly merge independently of this PR. It fixes a bug and also makes this work smaller.
Yes, I am definitely looking for suggestions on how many of the first commits it would make sense to split off into a separate PR.
But just to be clear, there isn't a bug here, just suboptimal behavior. If -reindex-chainstate is used and there are still block-connect
...
(https://github.com/bitcoin/bitcoin/pull/24230#discussion_r1284581060)
> Yeah ok, that is also a possibility. Great.
>
> Could the fix be decoupled into a standalone PR? Sounds like something that we could rapidly merge independently of this PR. It fixes a bug and also makes this work smaller.
Yes, I am definitely looking for suggestions on how many of the first commits it would make sense to split off into a separate PR.
But just to be clear, there isn't a bug here, just suboptimal behavior. If -reindex-chainstate is used and there are still block-connect
...
💬 theuni commented on pull request "[WIP] guix: use `-muse-unaligned-vector-move` for Windows builds":
(https://github.com/bitcoin/bitcoin/pull/28151#issuecomment-1665814980)
> > Any reason not to add it to our configure directly rather than guix?
>
> I think that's actually all we can do here, adding this (to configure) as a best-effort for anyone using an unpatched compiler (where it'd be most beneficial when building `bitcoind.exe`), while also retaining our GCC patching in Guix.
Are you sure those two aren't mutually exclusive? If so, yeah, that sounds reasonable. Though I think we should make an effort to [produce working binaries](https://github.com/bitc
...
(https://github.com/bitcoin/bitcoin/pull/28151#issuecomment-1665814980)
> > Any reason not to add it to our configure directly rather than guix?
>
> I think that's actually all we can do here, adding this (to configure) as a best-effort for anyone using an unpatched compiler (where it'd be most beneficial when building `bitcoind.exe`), while also retaining our GCC patching in Guix.
Are you sure those two aren't mutually exclusive? If so, yeah, that sounds reasonable. Though I think we should make an effort to [produce working binaries](https://github.com/bitc
...
💬 jesterhodl commented on pull request "Remove arbitrary restrictions on OP_RETURN by default":
(https://github.com/bitcoin/bitcoin/pull/28130#issuecomment-1665828005)
As a Bitcoin Node runner, Lightning Node operator, Nostr Relay operator and Nostr user, I recognize the problem @ChristopherA is looking to alleviate.
I recognize the problem is in the realm of authentication where cryptographic primitives and an immutable storage could be one option to pursue for a potential solution.
As a Bitcoiner I Concept NACK because:
* Timechain is for monetary data, other uses should forever stay as tolerable, low volume noise, or it will *decrease our capacity to
...
(https://github.com/bitcoin/bitcoin/pull/28130#issuecomment-1665828005)
As a Bitcoin Node runner, Lightning Node operator, Nostr Relay operator and Nostr user, I recognize the problem @ChristopherA is looking to alleviate.
I recognize the problem is in the realm of authentication where cryptographic primitives and an immutable storage could be one option to pursue for a potential solution.
As a Bitcoiner I Concept NACK because:
* Timechain is for monetary data, other uses should forever stay as tolerable, low volume noise, or it will *decrease our capacity to
...
💬 vasild commented on pull request "net: disconnect inside AttemptToEvictConnection":
(https://github.com/bitcoin/bitcoin/pull/27912#issuecomment-1665852012)
Yes! I was thinking about the same! We are doing some manual refcounting here which is creating all kinds of headaches and is prone to bugs. There is a neat solution already for this `std::shared_ptr`. Long term, this is a must have IMO.
To minimize the size of the initial PR, I guess you can just change `CConnman::m_nodes` and `CConnman::m_nodes_disconnected` from `std::vector<CNode*>` to `std::vector<CNodeRef>` and the needed changes to get that to compile and work.
Later I think we don'
...
(https://github.com/bitcoin/bitcoin/pull/27912#issuecomment-1665852012)
Yes! I was thinking about the same! We are doing some manual refcounting here which is creating all kinds of headaches and is prone to bugs. There is a neat solution already for this `std::shared_ptr`. Long term, this is a must have IMO.
To minimize the size of the initial PR, I guess you can just change `CConnman::m_nodes` and `CConnman::m_nodes_disconnected` from `std::vector<CNode*>` to `std::vector<CNodeRef>` and the needed changes to get that to compile and work.
Later I think we don'
...
💬 rot13maxi commented on pull request "Remove arbitrary restrictions on OP_RETURN by default":
(https://github.com/bitcoin/bitcoin/pull/28130#issuecomment-1665852027)
For readers of the thread who are not familiar with the STAMPS community, people are going around the OP_RETURN limit and doing things like embedding data in bare multisig: https://mempool.space/tx/1c61f2d4000e1bc2ce27ec24e4fcbbb376c2bd19641e071403e68d84d48eea8a
This creates many unprunable outputs and takes up more blockspace, so its bad for block capacity and utxoset bloat.
(https://github.com/bitcoin/bitcoin/pull/28130#issuecomment-1665852027)
For readers of the thread who are not familiar with the STAMPS community, people are going around the OP_RETURN limit and doing things like embedding data in bare multisig: https://mempool.space/tx/1c61f2d4000e1bc2ce27ec24e4fcbbb376c2bd19641e071403e68d84d48eea8a
This creates many unprunable outputs and takes up more blockspace, so its bad for block capacity and utxoset bloat.