💬 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.
💬 vasild commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1284610972)
_moving discussion from https://github.com/bitcoin/bitcoin/pull/27509#issuecomment-1659008138_
> 4. Finally, on signet I never got an INV for one of my own txs back. so getmempoolinfo : unbroadcastcount only ever increased. I notice that the tests only use python P2PInterface()'s for peers and the observer_outbound peer and is manually forced to inv our txid... makes me wonder if real bitcoind peers have different enough relay behavior that it doesnt work as well in production. For example, w
...
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1284610972)
_moving discussion from https://github.com/bitcoin/bitcoin/pull/27509#issuecomment-1659008138_
> 4. Finally, on signet I never got an INV for one of my own txs back. so getmempoolinfo : unbroadcastcount only ever increased. I notice that the tests only use python P2PInterface()'s for peers and the observer_outbound peer and is manually forced to inv our txid... makes me wonder if real bitcoind peers have different enough relay behavior that it doesnt work as well in production. For example, w
...