Concept ACK 👍

Concept ACK.

Are you planing on squashing everything to a single commit? Intermediate commits are broken (eg 15d3a6d3343f27e8c01faa5674a2fe406488eac3 breaks building script_error.cpp)

I think it is possible to do more of this as a scripted diff. I was looking into this this morning, and the following command seemed to work well for switching all include paths that don't depend on the current directory from "" to <> syntax:

for f in \
  src/*.cpp \
  src/*.h \
  src/bench/*.cpp \
  src/bench/*.h \
  src/compat/*.cpp \
  src/compat/*.h \
  src/consensus/*.cpp \
  src/consensus/*.h \
  src/crypto/*.cpp \
  src/crypto/*.h \
  src/crypto/ctaes/*.h \
  src/policy/*.cpp \
  src/policy/*.h \
  src/primitives/*.cpp \
  src/primitives/*.h \
  src/qt/*.cpp \
  src/qt/*.h \
  src/qt/test/*.cpp \
  src/qt/test/*.h \
  src/rpc/*.cpp \
  src/rpc/*.h \
  src/script/*.cpp \
  src/script/*.h \
  src/support/*.cpp \
  src/support/*.h \
  src/support/allocators/*.h \
  src/test/*.cpp \
  src/test/*.h \
  src/wallet/*.cpp \
  src/wallet/*.h \
  src/wallet/test/*.cpp \
  src/wallet/test/*.h \
  src/zmq/*.cpp \
  src/zmq/*.h
do
  base=${f%/*} sed -i "s:#include \"\(.*\)\"\(.*\):if test -e \$base/'\\1'; then echo '#include \"\\1\"\\2'; else echo '#include <\\1>\\2'; fi:e" $f
done

My branch is here (2 commits): https://github.com/ryanofsky/bitcoin/commits/pr/inc

It also updates developer notes. Feel free to take anything there that might be useful

Are you planing on squashing everything to a single commit? Intermediate commits are broken (eg 15d3a6d breaks building script_error.cpp)

Yes, before merging we could squash it. Before then, the current order of commits is easer to keep up to date with master.

I think it is possible to do more of this as a scripted diff

I'm not entirely sure how to rebase on top of your changes, or how to combine it.

My changes are more complete. For example I've changed qt/forms around too, as well as all the includes in src/qt. I think I completely switched over everything.

I've cherry-picked the developer notes change, thanks.

Great.
utACK cbe3ac57583a683a82a05ab93dec1fb4877ba5c4

@ryanofsky
I've adopted your script a bit and now it gets even closer:

  base=${f%/*}/ relbase=${base#src/} sed -i "s:#include \"\(.*\)\"\(.*\):if test -e \$base'\\1'; then echo '#include <'\$relbase'\\1>\\2'; else echo '#include <\\1>\\2'; fi:e" $f

Changed the base commit to modified @ryanofsky's script. The rest of the changes is much smaller now.

Also the individual commits should build because I moved the -I build system change to the end.

641d4c5 👏 utACK 895330f.

This makes me nervous without a subdir-as-a-namespace. gcc (i assume most others as well) searches -I's left-to-right, meaning that something with a generic name like "config.h" could end up including something from the system if we manage to add our "-I(builddir)" after some other path.

Testing with include <amount.h> for example, I was able to change MAX_MONEY by copying amount.h to /usr/include, and (to mimic an accidental ordering bug) re-arranging the -I's on the compile line. Trying that with include "amount.h" it at least got confused by the clashing files due to the primitives subdir.

Granted, that's pretty paranoid.

Much more likely if there was an ordering bug though, would be a compilation failure. A subdir would at least save us from those. Would it make sense to use -I$(top_builddir) and #include <src/amount.h> as a precaution?

This makes me nervous without a subdir-as-a-namespace. gcc (i assume most others as well) searches -I's left-to-right, meaning that something with a generic name like "config.h" could end up including something from the system if we manage to add our "-I(builddir)" after some other path.

But isn't that true for "" as well?
The only difference is that "" tries relative to the current directory first. It does not (IIRC) reverse -I order.
In the example that I give in the OP, primitives/block.h includes "primitives/transaction.h", it will look in src/primitives/primitives, failing that it goes down the entire -I chain, to finally try in src/.
So that would find /usr/include/primitives/transaction.h first.

Duplicated include files can lead to more confusion with relative includes as it can be ambiguous which one is included. At least with -I you can just follow the include path when figuring out the issue.

Much more likely if there was an ordering bug though, would be a compilation failure. A subdir would at least save us from those. Would it make sense to use -I$(top_builddir) and #include <src/amount.h> as a precaution?

I think on the longer run we should make sure all source files are in directories under src/, not src itself. Many bigger open source projects seem to have this convention.

I really dislike using top_builddir as an intermediate step though. It'd work, but feels dirty to add src/ before each include. I'd rather just close this.

Regarding active paranoia, someone could still put a src/amount.h in your include path and it will take precedence.

Only insanely disciplined relative inclusion (using explicit ./ and ../) could avoid that "attack", I think. That's... crazy though :( In any case if your include path is compromised, there are lots of other ways to create harm.

Would it make sense to use -I$(top_builddir) and #include <src/amount.h> as a precaution?

Do you know of any projects that do this?

Closing this for now, on further thought, this creates more issues than it solves I guess.

Closing this for now, on further thought, this creates more issues than it solves I guess.

Is this only being closed because of the issue theuni raised? Or are there other problems?

My earlier scripted diff (branch) is neutral with respect to the issue theuni is talking about because it leaves includes that are relative to the source directory intact, and only modifies includes that rely on the include path from <> to "". So for example, if an amount.h were added to an prepended include path, nothing different would happen than already happens now.

More broadly though, it is hard to imagine taking this issue seriously, because the project already uses a mix of <> and "" includes, and all of the existing <> includes (as well as nearly all the "" includes in subdirectories below src/) are already relying on the configured include paths. And as laanwj pointed out this change is only removing include paths, not adding any, so it is strictly decreasing whatever ambiguities there may be in interpreting #include directives compared to what we are doing now.

Is this only being closed because of the issue theuni raised? Or are there other problems?

Because of the issue theuni raised. I don't see any way out of that, he's right and we're already screwed, and this doesn't make it better (though it does make things more consistent, if they mess up! I have to stand by that. You won't have file A including the right copy and file B including the wrong one, for instance.).

So for example, if an amount.h were added to an prepended include path, nothing different would happen than already happens now.

I did think about that, and that would be true if we weren't using subdirectories ourselves. However as soon as we #include "amount.h" in one of the subdirectories of src, the same thing happens! This is what I mean with "insanely disciplined relative inclusion" two posts back. #include "../amount.h" would be unambiguous (if you do this consistently, I guess you could even remove -I completely for the project itself).

In Qt I think each module uses "" for self headers and <> for other module headers and everything else. But public headers always use <>.

if you do this consistently, I guess you could even remove -I completely for the project itself

I'm somewhat starting to like this idea. Make all includes relative and correct, then lose -I. What are the drawbacks?

Edit: To answer this myself, I guess things can become ambiguous if you have chains of include files including each other. E.g. "primitives/primitives.h" including "../amount.h" which includes "util.h". Which util.h would be included? Darn.

But public headers always use <>.

Yep, and we don't have any public headers besides bitcoinconsensus.h.

@laanwj I think it's too early to close this. I was thinking outloud about any potential drawbacks, but that's not to say that this creates more problems than it solves. As you mentioned, both of those potential issues are very unlikely.

As you mentioned, both of those potential issues are very unlikely.

Could someone state clearly what two potential issues this PR causes? I think I might have missed that there is more than one issue...

@ryanofsky

1. Potentially causing compilation issues with generic header names (util.h for example) if -I$(builddir) isn't first on the list. This is already an issue due to source files in subdirs, but the likelihood increases with this change.
2. Due to 1., potentially making it easier for someone in a shared dev environment to hijack a header with no indication. I don't think this is realistic to worry about because there are so many worse attack vectors. This was mainly just a thought experiment for testing 1.

Again, I'm not at all convinced that those things are real issues. Just thinking through them.

Thanks! Just to give my opinion, I think any of the following changes would be simple and minimally disruptive improvements on the status quo:

1. A change using -Isrc and #include<full_path> everywhere that would make the build slightly more vulnerable to theuni's issues than we already are, but would make it more obvious what file is being included in the normal case where you don't have a messed up build configuration. This is implemented in the current PR.

2. A change dropping -Isrc and using #include "../rel_path" everywhere that would prevent the issues raised by theuni in present and future code. This should not be hard to implement with a variation of laanwj's scripted diff.

3. A change that would not alter current behavior at all, but just improve readability by replacing current hodgepodge of #include "rel_path" and #include "full_path" with #include "rel_path" and #include <full_path>, to make it clear when the include path is supposed to be interpreted as a relative path, and when it's supposed to be interpreted as a full path. My pr/inc branch basically does this already but it would need a tweak to handle bitcoin-config.h.

Slight preference for (1) over (2) over (3), but I think any of these could be good improvements and there may be other simple solutions as well (like using -iquote or -I-).

If more disruptive changes are ok, it'd also be nice to move headers in the src/ root to appropriate subdirectories. But I think that type of change would be better to handle separately and should not block simple improvements like the one implemented in this PR.

This has acks from me, jonas, and promag, as well as concept acks from jnewbery and practicalswift and cory withdrew his objection. @laanwj can this be rebased and merged?

I think this is open for grabs

I think this is open for grabs

What does this mean? Maybe there should be a tag for PRs where author wants someone to take over or needs help with something.

Ah. I think I meant "this is up for grabs".

I'll rebase and open a new PR, I'm happy to maintain it since it already has so many ACKs

Closing now that #11651 is open.