💬 sipa commented on pull request "Use LE hex-encoded representations in script ASM for pushed values <= 4 bytes":
(https://github.com/bitcoin/bitcoin/pull/28824#issuecomment-1816445363)
Brainstormy idea for undecodable scripts.
These are necessarily pushes (direct or OP_PUSHDATA[124]) whose prescribed length exceeds the end of the script. What about using `<...+N>` or `OPCODE<...+N>`, where `...` is hexadecimal as before, and `N` is a decimal number indicating how many bytes are missing?
(https://github.com/bitcoin/bitcoin/pull/28824#issuecomment-1816445363)
Brainstormy idea for undecodable scripts.
These are necessarily pushes (direct or OP_PUSHDATA[124]) whose prescribed length exceeds the end of the script. What about using `<...+N>` or `OPCODE<...+N>`, where `...` is hexadecimal as before, and `N` is a decimal number indicating how many bytes are missing?
💬 hebasto commented on issue "ci: failure in Windows MSVC build":
(https://github.com/bitcoin/bitcoin/issues/28901#issuecomment-1816449938)
> Ok, I was asking because the latest build passed: [bitcoin/bitcoin/actions/runs/6903309087/job/18781736984](https://github.com/bitcoin/bitcoin/actions/runs/6903309087/job/18781736984) So I guess GitHub only pushes out the new image to some tasks?
It passed because the job was using the old image:
```
Runner Image
Image: windows-2022
Version: 20231029.1.0
```
The new version 20231115 is still problematic. The current rollout [progress](https://github.com/actions/runner-images#ava
...
(https://github.com/bitcoin/bitcoin/issues/28901#issuecomment-1816449938)
> Ok, I was asking because the latest build passed: [bitcoin/bitcoin/actions/runs/6903309087/job/18781736984](https://github.com/bitcoin/bitcoin/actions/runs/6903309087/job/18781736984) So I guess GitHub only pushes out the new image to some tasks?
It passed because the job was using the old image:
```
Runner Image
Image: windows-2022
Version: 20231029.1.0
```
The new version 20231115 is still problematic. The current rollout [progress](https://github.com/actions/runner-images#ava
...
💬 vasild commented on pull request "p2p: do not make automatic outbound connections to addnode peers":
(https://github.com/bitcoin/bitcoin/pull/28895#discussion_r1397327937)
(hit "submit" too early)
nit, non-blocker: I do not think that this `< 24` cap is needed. What is the max number of added nodes expected?
I think it is not worth doing that, but in theory, if this is to be optimized - then we can observe that `addr_port_str` begins with `addr_str`. For example `aaa:8333` and `aaa` and if we are checking if string `aaaa` is equal to either one, the way this is done now is that it would traverse the entire `aaaa` twice whereas it can be done with a single tr
...
(https://github.com/bitcoin/bitcoin/pull/28895#discussion_r1397327937)
(hit "submit" too early)
nit, non-blocker: I do not think that this `< 24` cap is needed. What is the max number of added nodes expected?
I think it is not worth doing that, but in theory, if this is to be optimized - then we can observe that `addr_port_str` begins with `addr_str`. For example `aaa:8333` and `aaa` and if we are checking if string `aaaa` is equal to either one, the way this is done now is that it would traverse the entire `aaaa` twice whereas it can be done with a single tr
...
💬 jonatack commented on pull request "p2p: do not make automatic outbound connections to addnode peers":
(https://github.com/bitcoin/bitcoin/pull/28895#discussion_r1397339786)
I think the maximum number of added nodes is up to the user. Hopefully we can change `m_added_node_params` to an unordered set as you suggested in #28248 to make the query constant-time on average. Kept it simple here.
(https://github.com/bitcoin/bitcoin/pull/28895#discussion_r1397339786)
I think the maximum number of added nodes is up to the user. Hopefully we can change `m_added_node_params` to an unordered set as you suggested in #28248 to make the query constant-time on average. Kept it simple here.
💬 willcl-ark commented on pull request "Use LE hex-encoded representations in script ASM for pushed values <= 4 bytes":
(https://github.com/bitcoin/bitcoin/pull/28824#issuecomment-1816491609)
> Brainstormy idea for undecodable scripts.
Currently failure to fully parse in `GetScriptOp` will only return `false` if we run out of bytes to read either the length, or from the operand, and we don't store the `opcode` or the available remaining bytes from the iterator in `opcodeRet` or `pvchRet`. But we could change `GetScriptOpt` to store them the failure case too.
> `<...+N>` or `OPCODE<...+N>`
I don't think it makes much difference which flavour is chosen here; both would require
...
(https://github.com/bitcoin/bitcoin/pull/28824#issuecomment-1816491609)
> Brainstormy idea for undecodable scripts.
Currently failure to fully parse in `GetScriptOp` will only return `false` if we run out of bytes to read either the length, or from the operand, and we don't store the `opcode` or the available remaining bytes from the iterator in `opcodeRet` or `pvchRet`. But we could change `GetScriptOpt` to store them the failure case too.
> `<...+N>` or `OPCODE<...+N>`
I don't think it makes much difference which flavour is chosen here; both would require
...
💬 sipa commented on pull request "Use LE hex-encoded representations in script ASM for pushed values <= 4 bytes":
(https://github.com/bitcoin/bitcoin/pull/28824#issuecomment-1816496295)
> I'd probably slightly lean towards using `OPCODE<...+N>` as it's slightly more human-friendly?
In case it's a direct push there is no opcode at all.
(https://github.com/bitcoin/bitcoin/pull/28824#issuecomment-1816496295)
> I'd probably slightly lean towards using `OPCODE<...+N>` as it's slightly more human-friendly?
In case it's a direct push there is no opcode at all.
📝 TheCharlatan opened a pull request: "refactor: Make CTxMemPoolEntry non-copyable"
(https://github.com/bitcoin/bitcoin/pull/28903)
This has the goal of prohibiting users from accidentally creating runtime failures, e.g. by interacting with iterator_to with a copied entry. This was brought up here: https://github.com/bitcoin/bitcoin/pull/28886#issuecomment-1814794954.
CTxMemPoolEntry is already implicitly not move-constructable. So be explicit about this and use a std::list to collect the values in the policy_estimator fuzz test instead of a std::vector.
(https://github.com/bitcoin/bitcoin/pull/28903)
This has the goal of prohibiting users from accidentally creating runtime failures, e.g. by interacting with iterator_to with a copied entry. This was brought up here: https://github.com/bitcoin/bitcoin/pull/28886#issuecomment-1814794954.
CTxMemPoolEntry is already implicitly not move-constructable. So be explicit about this and use a std::list to collect the values in the policy_estimator fuzz test instead of a std::vector.
🚀 fanquake merged a pull request: "doc: Simplify guix install doc, after 1.4 release"
(https://github.com/bitcoin/bitcoin/pull/28902)
(https://github.com/bitcoin/bitcoin/pull/28902)
📝 ajtowns opened a pull request: "Drop CAutoFile"
(https://github.com/bitcoin/bitcoin/pull/28904)
Continuing the move away from `GetVersion()`, replace uses of `CAutoFile` with `AutoFile`.
(https://github.com/bitcoin/bitcoin/pull/28904)
Continuing the move away from `GetVersion()`, replace uses of `CAutoFile` with `AutoFile`.
💬 ajtowns commented on pull request "refactor: P2P transport without serialize version and type":
(https://github.com/bitcoin/bitcoin/pull/28892#discussion_r1397392586)
Fair enough
(https://github.com/bitcoin/bitcoin/pull/28892#discussion_r1397392586)
Fair enough
💬 sipa commented on pull request "refactor: P2P transport without serialize version and type":
(https://github.com/bitcoin/bitcoin/pull/28892#issuecomment-1816546475)
utACK fade05b27348f18d1f9cf8c0b5cc26eaffe6b452
I'm also ok with a commit in this PR to drop `NetMsgMaker` entirely.
(https://github.com/bitcoin/bitcoin/pull/28892#issuecomment-1816546475)
utACK fade05b27348f18d1f9cf8c0b5cc26eaffe6b452
I'm also ok with a commit in this PR to drop `NetMsgMaker` entirely.
💬 fanquake commented on pull request "doc: remove mingw-w64 install for "older" systems":
(https://github.com/bitcoin/bitcoin/pull/28900#issuecomment-1816549218)
> a condition might be removed as well.
That wont work on systems that don't ship a `x86_64-w64-mingw32-g++` with the -posix suffix? i.e Alpine.
(https://github.com/bitcoin/bitcoin/pull/28900#issuecomment-1816549218)
> a condition might be removed as well.
That wont work on systems that don't ship a `x86_64-w64-mingw32-g++` with the -posix suffix? i.e Alpine.
💬 maflcko commented on pull request "refactor: P2P transport without serialize version and type":
(https://github.com/bitcoin/bitcoin/pull/28892#discussion_r1397423169)
I haven't tried it, but I presume that introducing the second template overload requires some header/module shuffling to avoid circular dependencies.
Happy to take a look, I just didn't want to step on your toes of the cnode/connection pull request (or anything else you are working on).
(https://github.com/bitcoin/bitcoin/pull/28892#discussion_r1397423169)
I haven't tried it, but I presume that introducing the second template overload requires some header/module shuffling to avoid circular dependencies.
Happy to take a look, I just didn't want to step on your toes of the cnode/connection pull request (or anything else you are working on).
💬 pablomartin4btc commented on pull request "Update Node window title with the chain type":
(https://github.com/bitcoin-core/gui/pull/758#issuecomment-1816563227)
Thanks @BrandonOdiwuor for testing it.
Main window is fine, this PR updates the title on the node window (e.g.: Main Menu->Window->Console).
> Is this a platform specific issue?
It should work on any platform.
(https://github.com/bitcoin-core/gui/pull/758#issuecomment-1816563227)
Thanks @BrandonOdiwuor for testing it.
Main window is fine, this PR updates the title on the node window (e.g.: Main Menu->Window->Console).
> Is this a platform specific issue?
It should work on any platform.
💬 maflcko commented on pull request "Drop CAutoFile":
(https://github.com/bitcoin/bitcoin/pull/28904#discussion_r1397433255)
nit: in e25bb1698f3a0f2ce095832202b3b4354ab3a08d: I don't think deleting this is correct, according to iwyu? My understanding is that it may be fine to have a return type in C++ forward declared without including the full header, as long as the function whose return type is forward declared is never called without having the full header. But I could be wrong?
(https://github.com/bitcoin/bitcoin/pull/28904#discussion_r1397433255)
nit: in e25bb1698f3a0f2ce095832202b3b4354ab3a08d: I don't think deleting this is correct, according to iwyu? My understanding is that it may be fine to have a return type in C++ forward declared without including the full header, as long as the function whose return type is forward declared is never called without having the full header. But I could be wrong?
💬 maflcko commented on pull request "Drop CAutoFile":
(https://github.com/bitcoin/bitcoin/pull/28904#discussion_r1397435169)
nit: This is related to the blockmanager commit, so could just squash 8a8d9d0a6ac6378b06c57911cf5a3c2b48a19610 into the previous commit?
(https://github.com/bitcoin/bitcoin/pull/28904#discussion_r1397435169)
nit: This is related to the blockmanager commit, so could just squash 8a8d9d0a6ac6378b06c57911cf5a3c2b48a19610 into the previous commit?
👍 maflcko approved a pull request: "Drop CAutoFile"
(https://github.com/bitcoin/bitcoin/pull/28904#pullrequestreview-1737188965)
Nice
ACK 6b70cf77b5beda5bd25246aefed2633410d4126f 🚾
<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: ACK 6b70cf77b5beda5bd2
...
(https://github.com/bitcoin/bitcoin/pull/28904#pullrequestreview-1737188965)
Nice
ACK 6b70cf77b5beda5bd25246aefed2633410d4126f 🚾
<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: ACK 6b70cf77b5beda5bd2
...
👍 guggero approved a pull request: "p2p: do not make automatic outbound connections to addnode peers"
(https://github.com/bitcoin/bitcoin/pull/28895#pullrequestreview-1737192944)
utACK 5e7cc4144bb09cfde48ba5bc055a2da3ca4252d1
I think it's important that nodes added by `addnode` always get the elevated protection level.
> A completely unrelated problem is that this problem exists because cjdns has so few peers <10? on mainnet, which is a shame considering how much engineering effort went into it, but I'm not sure what to do about that.
I'm still trying to wrap my head around CJDNS and how it can be set up in a docker based environment. If successful, I'll try cre
...
(https://github.com/bitcoin/bitcoin/pull/28895#pullrequestreview-1737192944)
utACK 5e7cc4144bb09cfde48ba5bc055a2da3ca4252d1
I think it's important that nodes added by `addnode` always get the elevated protection level.
> A completely unrelated problem is that this problem exists because cjdns has so few peers <10? on mainnet, which is a shame considering how much engineering effort went into it, but I'm not sure what to do about that.
I'm still trying to wrap my head around CJDNS and how it can be set up in a docker based environment. If successful, I'll try cre
...
💬 hebasto commented on issue "v26.0 Testing":
(https://github.com/bitcoin/bitcoin/issues/28718#issuecomment-1816644217)
> > All binaries are downloaded from [bitcoincore.org/bin](https://bitcoincore.org/bin/).
>
> Does it also happen if you compile from source?
It does.
> If yes, you could try bisecting (without having to go through guix builds)
Due to unknown for reason natively compiled binaries, at least `bitcoind -reindex-chainstate`, are significantly slower than Guix built ones. Therefore, I'm in the middle of bisecting using Guix builds.
(https://github.com/bitcoin/bitcoin/issues/28718#issuecomment-1816644217)
> > All binaries are downloaded from [bitcoincore.org/bin](https://bitcoincore.org/bin/).
>
> Does it also happen if you compile from source?
It does.
> If yes, you could try bisecting (without having to go through guix builds)
Due to unknown for reason natively compiled binaries, at least `bitcoind -reindex-chainstate`, are significantly slower than Guix built ones. Therefore, I'm in the middle of bisecting using Guix builds.
💬 ajtowns commented on pull request "refactor: P2P transport without serialize version and type":
(https://github.com/bitcoin/bitcoin/pull/28892#issuecomment-1816647954)
ACK fade05b27348f18d1f9cf8c0b5cc26eaffe6b452
(https://github.com/bitcoin/bitcoin/pull/28892#issuecomment-1816647954)
ACK fade05b27348f18d1f9cf8c0b5cc26eaffe6b452