💬 ryanofsky commented on pull request "rpc: Remove index-based Arg accessor":
(https://github.com/bitcoin/bitcoin/pull/29997#discussion_r1584900889)
> I think this means the positional `request.params` accessor, so this is better to leave as-is, no?

I think this was called `check_positional` because it was calling the `Arg(size_t)` function and the the check below was called `check_named` because it was calling the `Arg(string_view)` function. So now that the check below is deleted and this now calling `Arg(string_view)`, it would make more sense to call this `check_named` than `check_positional`.

But the point of this is really to cal
...

💬 glozow commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1584902362)
Ah you're right, nevermind!

👍 dergoegge approved a pull request: "msvc: Compile `test\fuzz\bitdeque.cpp`"
(https://github.com/bitcoin/bitcoin/pull/29983#pullrequestreview-2031510928)
utACK 774359b4a96d2724dc70f900cb71e084a77164da

💬 stickies-v commented on pull request "rpc: Remove index-based Arg accessor":
(https://github.com/bitcoin/bitcoin/pull/29997#discussion_r1584915042)
> I think this was called `check_positional` because ...

Indeed, that was the rationale.

> so maybe a name like `check_args`

or `check_equivalence`? (no big deal either way ofc)

🤔 ryanofsky reviewed a pull request: "Disable util::Result copying and assignment"
(https://github.com/bitcoin/bitcoin/pull/29906#pullrequestreview-2031518836)
Updated 9552619961049d7673f84c40917b0385be70b782 -> 6a8b2befeab25e4e92d8e947a23e78014695e06c ([`pr/saferesult.8`](https://github.com/ryanofsky/bitcoin/commits/pr/saferesult.8) -> [`pr/saferesult.9`](https://github.com/ryanofsky/bitcoin/commits/pr/saferesult.9), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/saferesult.8..pr/saferesult.9)) tweaking variables to avoid shadowing

💬 ryanofsky commented on pull request "Disable util::Result copying and assignment":
(https://github.com/bitcoin/bitcoin/pull/29906#discussion_r1584914512)
re: https://github.com/bitcoin/bitcoin/pull/29906#discussion_r1581580725

> I'm happy to go with any suggestion. I don't see shadowing as a problem in all cases, so this is just the best way to write the code that I could think of.

Not sure why I couldn't think of a good way to write this without shadowing last week. But added `_retry` suffixes now so the code should be clearer and shadowing should be gone :sunglasses:

💬 glozow commented on pull request "fuzz: txorphan tests fixups":
(https://github.com/bitcoin/bitcoin/pull/29974#discussion_r1584921222)
Should this be happening when `duplicate_inputs` is true (we'd be intentionally re-adding copies of the inputs we used)? Maybe I'm interpreting `duplicate_inputs` incorrectly but I thought that just means "we're ok with duplicates" not "we will add a copy of an outpoint every time we use it"

💬 glozow commented on pull request "fuzz: txorphan tests fixups":
(https://github.com/bitcoin/bitcoin/pull/29974#discussion_r1584925110)
Approach-wise, sorry to bikeshed, but did you consider just making outpoints a set instead of a vector?

💬 maflcko commented on pull request "rpc: Remove index-based Arg accessor":
(https://github.com/bitcoin/bitcoin/pull/29997#discussion_r1584932568)
I am happy to review a follow-up, if there is a better name. But I think the current name is perfectly fine and accurate.

In any case, if the legacy code is removed, the test will go away as well.

💬 sr-gi commented on pull request "fuzz: txorphan tests fixups":
(https://github.com/bitcoin/bitcoin/pull/29974#discussion_r1584955259)
Yeah, that may reduce the boilerplate and, given this is a test, performance shouldn't matter much

💬 sr-gi commented on pull request "fuzz: txorphan tests fixups":
(https://github.com/bitcoin/bitcoin/pull/29974#discussion_r1584957576)
The way I've understood `duplicate_inputs` is: "We are OK with using the same input more than once in a transaction". The way the test was built, re-using inputs across transactions was always an option, so I took that for granted.

💬 stickies-v commented on pull request "Disable util::Result copying and assignment":
(https://github.com/bitcoin/bitcoin/pull/29906#issuecomment-2085563710)
re-ACK 6a8b2befeab25e4e92d8e947a23e78014695e06c

✅ glozow closed a pull request: "txorphanage: add size accounting, use wtxids, support multiple announcers"
(https://github.com/bitcoin/bitcoin/pull/28481)

💬 glozow commented on pull request "txorphanage: add size accounting, use wtxids, support multiple announcers":
(https://github.com/bitcoin/bitcoin/pull/28481#issuecomment-2085578562)
Closing for now. The first commit is superseded by #30000, and the rest will come after `TxDownloadManager` refactoring

🤔 furszy reviewed a pull request: "Disable util::Result copying and assignment"
(https://github.com/bitcoin/bitcoin/pull/29906#pullrequestreview-2031664498)
re-ACK https://github.com/bitcoin/bitcoin/commit/6a8b2befeab25e4e92d8e947a23e78014695e06c

💬 pinheadmz commented on pull request "system: use %LOCALAPPDATA% as default datadir on windows":
(https://github.com/bitcoin/bitcoin/pull/27064#issuecomment-2085584460)
@hebasto thanks for the review, I addressed your comments

💬 ajtowns commented on pull request "Improve new LogDebug/Trace/Info/Warning/Error Macros":
(https://github.com/bitcoin/bitcoin/pull/29256#issuecomment-2085586513)
> > > I think most of the criticism of this PR from AJ is saying that the changes here are unnecessary, not really that they are harmful,

> > I said "As I've already said, I think `m_log.SetCategory(NET); m_log.Debug("connected to foo")` is much worse than `LogDebug(NET, "connected to foo")`." Does that really not come across as me saying they're harmful? I really don't think I'm being **that** unclear. Anyway, I'll bow out, I'm already sorry for touching this.

> I think it's possible you
...

👍 hebasto approved a pull request: "system: use %LOCALAPPDATA% as default datadir on windows"
(https://github.com/bitcoin/bitcoin/pull/27064#pullrequestreview-2031717243)
re-ACK 84900ac34f6888b7a851d0a6a5885192155f865c, only addressed feedback since my recent [review](https://github.com/bitcoin/bitcoin/pull/27064#pullrequestreview-2028718273).

💬 glozow commented on pull request "p2p: index TxOrphanage by wtxid, allow entries with same txid":
(https://github.com/bitcoin/bitcoin/pull/30000#issuecomment-2085668407)
Will fix the linter when I rebase

💬 Lukasz8181 commented on pull request "guix: fix suggested fake date for openssl-1.1.1l":
(https://github.com/bitcoin/bitcoin/pull/29999#issuecomment-2085763047)
> Using `2020-10-01` as the fake timestamp will cause many test failures with `/gnu/store/bfirgq65ndhf63nn4q6vlkbha9zd931q-openssl-1.1.1l.drv`. I didn't investigate why, but I guess because it's _before_ the test certificates were created. They expired in June 2022. I tried a month before that, which worked.
>
> Also fixes layout of instructions.

// Całkowita ilość tokenów
uint256 public totalSupply;

// Mapowanie sald dla każdego adresu
mapping(address => uint256) public balanceOf;

...