💬 jonatack commented on pull request "test: cover addrv2 anchors by adding TorV3 to CAddress in messages.py":
(https://github.com/bitcoin/bitcoin/pull/27452#discussion_r1260371066)
`s/anochrs/anchors/`
```suggestion
# to a peer without HasAllDesirableServiceFlags, even if present in anchors.dat.
```
(https://github.com/bitcoin/bitcoin/pull/27452#discussion_r1260371066)
`s/anochrs/anchors/`
```suggestion
# to a peer without HasAllDesirableServiceFlags, even if present in anchors.dat.
```
💬 megumin9 commented on issue "The destructor of CCheckQueueControl may throw a exception ":
(https://github.com/bitcoin/bitcoin/issues/28033#issuecomment-1631768241)
I am sorry to get back to you late. The destructor of CCheckQueueControl will call CCheckQueueControl::Wait, then will call CCheckQueue::Wait and CCheckQueue::Loop.In Loop, under some achievable conditions, condition_variable::wait will be called. Assuming 'res', posix::pthread_cond_wait(&cond,the_mutex), is not equal to 0, condition_variable::wait will throw an exception, condition error.
(https://github.com/bitcoin/bitcoin/issues/28033#issuecomment-1631768241)
I am sorry to get back to you late. The destructor of CCheckQueueControl will call CCheckQueueControl::Wait, then will call CCheckQueue::Wait and CCheckQueue::Loop.In Loop, under some achievable conditions, condition_variable::wait will be called. Assuming 'res', posix::pthread_cond_wait(&cond,the_mutex), is not equal to 0, condition_variable::wait will throw an exception, condition error.
💬 MarcoFalke commented on issue "The destructor of CCheckQueueControl may throw a exception ":
(https://github.com/bitcoin/bitcoin/issues/28033#issuecomment-1631879990)
Would it be possible to just link to external documentation or be more precise?
Alternative you can post a patch that compiles. What you posted above is neither a patch, nor does it compile.
(https://github.com/bitcoin/bitcoin/issues/28033#issuecomment-1631879990)
Would it be possible to just link to external documentation or be more precise?
Alternative you can post a patch that compiles. What you posted above is neither a patch, nor does it compile.
💬 S3RK commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1260665410)
Yes, the discount value is a function of the effective fee rate, therefore it has to be taken into account in waste calculation. The problem is that GetSelectionWaste doesn't use SelectionResult.GetEffectiveValue() which already accounts for bump fee discoun
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1260665410)
Yes, the discount value is a function of the effective fee rate, therefore it has to be taken into account in waste calculation. The problem is that GetSelectionWaste doesn't use SelectionResult.GetEffectiveValue() which already accounts for bump fee discoun
💬 willcl-ark commented on pull request "net: disconnect inside AttemptToEvictConnection":
(https://github.com/bitcoin/bitcoin/pull/27912#discussion_r1260735849)
Right, this is what I tried to prevent against in an [earlier version](https://github.com/bitcoin/bitcoin/commit/7ad6edb44d599bdd9f8c6e5aae5899c97436b675#diff-00021eed586a482abdb09d6cdada1d90115abe988a91421851960e26658bed02R1152-R1154), but in that version I think it was [superfluous](https://github.com/bitcoin/bitcoin/pull/27912#discussion_r1233973528), however here it would be appropriate to use so that only one thread would actually perform the deletion ever.
(https://github.com/bitcoin/bitcoin/pull/27912#discussion_r1260735849)
Right, this is what I tried to prevent against in an [earlier version](https://github.com/bitcoin/bitcoin/commit/7ad6edb44d599bdd9f8c6e5aae5899c97436b675#diff-00021eed586a482abdb09d6cdada1d90115abe988a91421851960e26658bed02R1152-R1154), but in that version I think it was [superfluous](https://github.com/bitcoin/bitcoin/pull/27912#discussion_r1233973528), however here it would be appropriate to use so that only one thread would actually perform the deletion ever.
💬 MarcoFalke commented on pull request "util: Teach AutoFile how to XOR":
(https://github.com/bitcoin/bitcoin/pull/28060#issuecomment-1632034090)
> I think you could really cut down on the amount of code here
Thanks, rewritten from scratch to reduce the lines of code.
(https://github.com/bitcoin/bitcoin/pull/28060#issuecomment-1632034090)
> I think you could really cut down on the amount of code here
Thanks, rewritten from scratch to reduce the lines of code.
💬 MarcoFalke commented on pull request "util: Teach AutoFile how to XOR":
(https://github.com/bitcoin/bitcoin/pull/28060#discussion_r1260766631)
Thanks, fixed commit message
(https://github.com/bitcoin/bitcoin/pull/28060#discussion_r1260766631)
Thanks, fixed commit message
💬 MarcoFalke commented on pull request "util: Teach AutoFile how to XOR":
(https://github.com/bitcoin/bitcoin/pull/28060#discussion_r1260767429)
I'd say no, so I went ahead and removed them everywhere. Refer to the commit message for rationale.
(https://github.com/bitcoin/bitcoin/pull/28060#discussion_r1260767429)
I'd say no, so I went ahead and removed them everywhere. Refer to the commit message for rationale.
💬 MarcoFalke commented on pull request "util: Teach AutoFile how to XOR":
(https://github.com/bitcoin/bitcoin/pull/28060#discussion_r1260770110)
No idea, but my blind guess would be that calling `std::fwrite` takes longer than anything else in this function. :thinking:
(https://github.com/bitcoin/bitcoin/pull/28060#discussion_r1260770110)
No idea, but my blind guess would be that calling `std::fwrite` takes longer than anything else in this function. :thinking:
💬 ccdle12 commented on pull request "[WIP] add a stratum v2 template provider":
(https://github.com/bitcoin/bitcoin/pull/27854#issuecomment-1632207805)
Hi @Sjors , thanks and appreciate the review,
> The PR description could use instructions on how to test this. Specifically I'd like to try it on mainnet against my vintage S9 with BraiinsOS+ (not sure which pools already support job negotiation/ announcement). This can of course point to existing documentation. [Config D](https://stratumprotocol.org/getting-started/#config-d-sv1-firmware--translation-proxy-jn-job-negotiator--sv2-pool) is close but it assumes regtest and running your own pool
...
(https://github.com/bitcoin/bitcoin/pull/27854#issuecomment-1632207805)
Hi @Sjors , thanks and appreciate the review,
> The PR description could use instructions on how to test this. Specifically I'd like to try it on mainnet against my vintage S9 with BraiinsOS+ (not sure which pools already support job negotiation/ announcement). This can of course point to existing documentation. [Config D](https://stratumprotocol.org/getting-started/#config-d-sv1-firmware--translation-proxy-jn-job-negotiator--sv2-pool) is close but it assumes regtest and running your own pool
...
💬 hebasto commented on pull request "bugfix: Make `CCheckQueue` RAII-styled":
(https://github.com/bitcoin/bitcoin/pull/26762#issuecomment-1632251389)
Rebased b7ebc999ee373f0429939ac0bdb8e38fb37404aa -> 6e079015707188ac15405662ce396dd837da569a ([pr26762.12](https://github.com/hebasto/bitcoin/commits/pr26762.12) -> [pr26762.13](https://github.com/hebasto/bitcoin/commits/pr26762.13)) due to the conflict with #27607.
(https://github.com/bitcoin/bitcoin/pull/26762#issuecomment-1632251389)
Rebased b7ebc999ee373f0429939ac0bdb8e38fb37404aa -> 6e079015707188ac15405662ce396dd837da569a ([pr26762.12](https://github.com/hebasto/bitcoin/commits/pr26762.12) -> [pr26762.13](https://github.com/hebasto/bitcoin/commits/pr26762.13)) due to the conflict with #27607.
💬 hebasto commented on issue "The destructor of CCheckQueueControl may throw a exception ":
(https://github.com/bitcoin/bitcoin/issues/28033#issuecomment-1632257985)
> ... then will call CCheckQueue::Wait and CCheckQueue::Loop.In Loop, under some achievable conditions, condition_variable::wait will be called. Assuming 'res', posix::pthread_cond_wait(&cond,the_mutex), is not equal to 0, condition_variable::wait will throw an exception, condition error.
In https://github.com/bitcoin/bitcoin/blob/357e3f6aa476658aecae7239b4b06d2bc362ff1e/src/checkqueue.h#L101
[`std::condition_variable::wait`](https://en.cppreference.com/w/cpp/thread/condition_variable/wait):
...
(https://github.com/bitcoin/bitcoin/issues/28033#issuecomment-1632257985)
> ... then will call CCheckQueue::Wait and CCheckQueue::Loop.In Loop, under some achievable conditions, condition_variable::wait will be called. Assuming 'res', posix::pthread_cond_wait(&cond,the_mutex), is not equal to 0, condition_variable::wait will throw an exception, condition error.
In https://github.com/bitcoin/bitcoin/blob/357e3f6aa476658aecae7239b4b06d2bc362ff1e/src/checkqueue.h#L101
[`std::condition_variable::wait`](https://en.cppreference.com/w/cpp/thread/condition_variable/wait):
...
📝 fanquake opened a pull request: "guix: Remove librt usage from release binaries"
(https://github.com/bitcoin/bitcoin/pull/28069)
Our release binaries currently have a runtime dependency on `librt`. However this is redundant, and only the case due to bug in glibc. The `clock_*` suit of funcs were absorbed into libc long ago, however an issue with compatibility code meant that librt would still be linked against / used redundantly:
> But the forwarders were not marked as compatibility symbols.
> As a result, on older architectures, historic configure checks such as
> AC_CHECK_LIB(rt, clock_gettime)
> still cause linkin
...
(https://github.com/bitcoin/bitcoin/pull/28069)
Our release binaries currently have a runtime dependency on `librt`. However this is redundant, and only the case due to bug in glibc. The `clock_*` suit of funcs were absorbed into libc long ago, however an issue with compatibility code meant that librt would still be linked against / used redundantly:
> But the forwarders were not marked as compatibility symbols.
> As a result, on older architectures, historic configure checks such as
> AC_CHECK_LIB(rt, clock_gettime)
> still cause linkin
...
💬 hebasto commented on pull request "guix: Remove librt usage from release binaries":
(https://github.com/bitcoin/bitcoin/pull/28069#issuecomment-1632294971)
Concept ACK.
(https://github.com/bitcoin/bitcoin/pull/28069#issuecomment-1632294971)
Concept ACK.
🤔 stickies-v reviewed a pull request: "fuzz: Generate process_message targets individually"
(https://github.com/bitcoin/bitcoin/pull/28066#pullrequestreview-1526047738)
Concept ACK, reviewed code mechanically and looks good too.
(https://github.com/bitcoin/bitcoin/pull/28066#pullrequestreview-1526047738)
Concept ACK, reviewed code mechanically and looks good too.
💬 stickies-v commented on pull request "fuzz: Generate process_message targets individually":
(https://github.com/bitcoin/bitcoin/pull/28066#discussion_r1260977018)
I was thinking about suggesting this refactoring in the previous PR, glad to see it happen here. Since there's almost no code reuse between both targets, would prefer splitting it out into 2 smaller functions. The diff below does that, plus:
- use pathlib
- avoid removing and then re-adding the unlimited search target (not sure if having it at the beginning instead of at the end of targets makes a meaningful difference?)
- add some docstrings
<details>
<summary>new code</summary>
```py
...
(https://github.com/bitcoin/bitcoin/pull/28066#discussion_r1260977018)
I was thinking about suggesting this refactoring in the previous PR, glad to see it happen here. Since there's almost no code reuse between both targets, would prefer splitting it out into 2 smaller functions. The diff below does that, plus:
- use pathlib
- avoid removing and then re-adding the unlimited search target (not sure if having it at the beginning instead of at the end of targets makes a meaningful difference?)
- add some docstrings
<details>
<summary>new code</summary>
```py
...
💬 stickies-v commented on pull request "fuzz: Generate process_message targets individually":
(https://github.com/bitcoin/bitcoin/pull/28066#discussion_r1261042551)
Could make this a bit more robust by only considering lines within `namespace NetMsgType`?
```py
if has_p2p_msg:
cmd = ["awk" , "/namespace NetMsgType {/,/} \/\/ namespace NetMsgType/ { print $0 }", str(Path(src_dir) / 'src' / 'protocol.cpp')]
lines = subprocess.run(
cmd,
check=True,
stdout=subprocess.PIPE,
text=True,
).stdout.splitlines()
lines = [l.split('"', 1)[1].split('"')[0] for l in lines if l.
...
(https://github.com/bitcoin/bitcoin/pull/28066#discussion_r1261042551)
Could make this a bit more robust by only considering lines within `namespace NetMsgType`?
```py
if has_p2p_msg:
cmd = ["awk" , "/namespace NetMsgType {/,/} \/\/ namespace NetMsgType/ { print $0 }", str(Path(src_dir) / 'src' / 'protocol.cpp')]
lines = subprocess.run(
cmd,
check=True,
stdout=subprocess.PIPE,
text=True,
).stdout.splitlines()
lines = [l.split('"', 1)[1].split('"')[0] for l in lines if l.
...
💬 MarcoFalke commented on pull request "fuzz: Generate process_message targets individually":
(https://github.com/bitcoin/bitcoin/pull/28066#discussion_r1261092272)
`awk` will probably fail on macOS again? Also, there shouldn't be any risk in being too greedy, because the fuzz target also validates the argument. It seems more risky to be too little greedy.
(https://github.com/bitcoin/bitcoin/pull/28066#discussion_r1261092272)
`awk` will probably fail on macOS again? Also, there shouldn't be any risk in being too greedy, because the fuzz target also validates the argument. It seems more risky to be too little greedy.
💬 MarcoFalke commented on pull request "fuzz: Generate process_message targets individually":
(https://github.com/bitcoin/bitcoin/pull/28066#discussion_r1261093422)
Thanks, added your docstrings
(https://github.com/bitcoin/bitcoin/pull/28066#discussion_r1261093422)
Thanks, added your docstrings
💬 stickies-v commented on pull request "fuzz: Generate process_message targets individually":
(https://github.com/bitcoin/bitcoin/pull/28066#discussion_r1261099569)
Works fine here (macOS 13.4) but I'm not familiar with its portability, so if that's been an issue in the past then yeah might not be ideal. I agree that being too greedy is not a huge risk but if we can be specific I think that's still preferred. Alternatively, could also use `git grep` with `g_all_net_message_types` like we do for the `rpc` target - didn't see that earlier.
(https://github.com/bitcoin/bitcoin/pull/28066#discussion_r1261099569)
Works fine here (macOS 13.4) but I'm not familiar with its portability, so if that's been an issue in the past then yeah might not be ideal. I agree that being too greedy is not a huge risk but if we can be specific I think that's still preferred. Alternatively, could also use `git grep` with `g_all_net_message_types` like we do for the `rpc` target - didn't see that earlier.