🤔 mzumsande reviewed a pull request: "Bugfix: net_processing: Restore "Already requested" error for FetchBlock"
(https://github.com/bitcoin/bitcoin/pull/28055#pullrequestreview-1525047228)
> The result of this PR would be that it would return an error IFF that particular block was requested by that specific peer last. If you request the block from another peer, the behavior would be unchanged. Is that acceptable?

I think that I would find that acceptable.
I guess one alternative would be to drop the "Already requested from this peer" error that is currently not triggerable, and just forget about the prior request unconditionally on a new request.

💬 achow101 commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1260253320)
In 94d02376c853aa4835de413d3f75b8b06bcb5193 "rpc: JSON-RPC 2.0 should not respond to "notifications""

This results in 500 and a response body of "Unhandled request". I think it should actually be a 200 with no response body.

```suggestion
if (jreq.IsNotification()) {
req->WriteReply(HTTP_OK);
return true;
}
```

🤔 jamesob reviewed a pull request: "util: Add XorFile"
(https://github.com/bitcoin/bitcoin/pull/28060#pullrequestreview-1525080853)
Concept ACK

In general looks good, but I think you could really cut down on the amount of code here if you just made `XorFile` a subclass of `CAutoFile`, and made whatever small adjustments and modernizations necessary to `CAutoFile` to get that to work.

E.g. `XorFile::ignore()` seems like essentially the same code as `CAutoFile::ignore()`, just with some `std::` prefixes. Seems like code we don't want to maintain two copies of.

💬 jamesob commented on pull request "util: Add XorFile":
(https://github.com/bitcoin/bitcoin/pull/28060#discussion_r1260227191)
fa2276355889ff7bcad95aa978863107c36a89f7

Minor: new function is called `detail_fread` but commit and commit message mention `detail_read`.

💬 jamesob commented on pull request "util: Add XorFile":
(https://github.com/bitcoin/bitcoin/pull/28060#discussion_r1260260590)
fa9325a14584be1c5aa6b4cdefbdaa69b27e402d

Might be interesting to see how statically allocating a buffer (say, as a member of `XorFile`) and reusing it might affect performance; may avoid some reallocations. Not sure how frequently this is actually called though. I expect this probably won't make a difference though, because I guess `XorFile::write()` will be called on the order of once per block.

💬 jamesob commented on pull request "util: Add XorFile":
(https://github.com/bitcoin/bitcoin/pull/28060#discussion_r1260249626)
fa9325a14584be1c5aa6b4cdefbdaa69b27e402d

Should these serialization methods have similar `if (!m_file)` guards as `AutoFile` does?

💬 jamesob commented on pull request "util: Add XorFile":
(https://github.com/bitcoin/bitcoin/pull/28060#discussion_r1260252291)
fa9325a14584be1c5aa6b4cdefbdaa69b27e402d

Why not throw a `std::ios_base::failure()` here? From what I can tell, we'd never expect to get back a negative error value.

💬 achow101 commented on pull request "test: refactor: deduplicate legacy ECDSA signing for tx inputs":
(https://github.com/bitcoin/bitcoin/pull/28025#issuecomment-1631522931)
ACK 5cf44275c8ca8c32d238f37f717d78e9823f44c2

🚀 achow101 merged a pull request: "test: refactor: deduplicate legacy ECDSA signing for tx inputs"
(https://github.com/bitcoin/bitcoin/pull/28025)

💬 theuni commented on issue "Use semantic analysis in lint-logs.py":
(https://github.com/bitcoin/bitcoin/issues/27825#issuecomment-1631605962)
Sure. Now that I think about it, we could just keep|copy the test suite in Core rather than|in addition to the plugin repo.

🤔 jonatack reviewed a pull request: "test: cover addrv2 anchors by adding TorV3 to CAddress in messages.py"
(https://github.com/bitcoin/bitcoin/pull/27452#pullrequestreview-1525275447)
ACK a5c9e059b7f463f668fa404dfc652791c481fd95

💬 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.
```

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

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

💬 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

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

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

💬 MarcoFalke commented on pull request "util: Teach AutoFile how to XOR":
(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.

💬 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: